Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update fiddler agent to replace deprecated v2 APIs with faster v3 APIs (ECOINT-84) #2592

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update fiddler agent to replace deprecated v2 APIs with faster v3 APIs (ECOINT-84) #2592

wants to merge 4 commits into from

Conversation

nikhil-singh-01
Copy link

@nikhil-singh-01 nikhil-singh-01 commented Jan 28, 2025 •
edited
Loading

What does this PR do?
This PR rewrites Fiddler's Agent to use new faster v3 APIs. The PR ensures that the results published to DataDog continue to be backwards compatible supporting old metrics names.
Changes:

  • Replace v2 APIs - /models, /metrics and /queries with v3 APIs and remove usage of v2/list-projects API.
  • Maintain backward compatibility for old metrics names(using v1compat flag) and add another dashboard for newer metric names.
  • Add new metric names to metadata.csv
  • Update spec.yaml to make FiddlerCheck more configurable.
  • Update and add tests for v3 APIs and v1compat flag turned off.

Motivation
What inspired you to submit this pull request?
Functional Reasons:

The existing implementation of Fiddler-Datadog integration uses v2 APIs - list_projects, and v2/queries and metrics
These APIs are deprecated and removed once users are successfully migrated.
These APIs are replaced by v3 APIs which have been in production for 10+ months. These APIs make much more efficient use of caching at every layer, and are faster.

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
  • If this PR includes a log pipeline, please add a description describing the remappers and processors.

Additional Notes

Anything else we should know when reviewing?

@nikhil-singh-01 nikhil-singh-01 marked this pull request as ready for review January 28, 2025 15:08
@nikhil-singh-01 nikhil-singh-01 requested a review from a team as a code owner January 28, 2025 15:08
bgoldberg122
bgoldberg122 requested changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@bgoldberg122 bgoldberg122 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left 1 comment, otherwise looks good to me

fiddler/manifest.json Outdated Show resolved Hide resolved
Kyle-Neale
Kyle-Neale reviewed Jan 28, 2025
View reviewed changes
Copy link
Contributor

@Kyle-Neale Kyle-Neale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we're dropping support for V2 API with these changes. Is this accurate?

fiddler/CHANGELOG.md Outdated

***Changed***:

* Changed to using v3 queries and metrics APIs. ([#2582](https://github.com/DataDog/integrations-extras/pull/2582))
Copy link
Contributor

@Kyle-Neale Kyle-Neale Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also update the PR to the current PR number.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching it. Fixed it.

@nikhil-singh-01
Copy link
Author

It seems like we're dropping support for V2 API with these changes. Is this accurate?

@Kyle-Neale Our backend v2 APIs are deprecated and we are removing it's usage from here. There's no breaking change for users.

@dd-dominic dd-dominic changed the title Update fiddler agent to replace deprecated v2 APIs with faster v3 APIs Update fiddler agent to replace deprecated v2 APIs with faster v3 APIs (ECOINT-84) Jan 29, 2025
@dd-dominic dd-dominic self-assigned this Jan 30, 2025
dd-dominic
dd-dominic requested changes Jan 30, 2025
View reviewed changes
Copy link
Collaborator

@dd-dominic dd-dominic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Requesting dashboard updates
  • Readme installation command also needs to be updated to reference 4.0.0

fiddler/assets/dashboards/fiddler_overview_v2.json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the dashboard file names get mixed up? The file name indicates v2, but dashboard name is "Fiddler Model Performance 3.0.0".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also fix grammar in description. This is present in both dashboards.

Current:

"This dashboard displays the model performance metrics that was pulled into DataDog from the Fiddler Model Performance Management tool."

Should be:

"This dashboard displays the model performance metrics that were pulled into DataDog from the Fiddler Model Performance Management tool."

fiddler/assets/dashboards/fiddler_overview_v3.json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the new dashboard, please update it to align with our template as closely as possible.
Integration_Dashboard_Template.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dd-dominic dd-dominic dd-dominic requested changes

@bgoldberg122 bgoldberg122 Awaiting requested review from bgoldberg122

@Kyle-Neale Kyle-Neale Awaiting requested review from Kyle-Neale

Requested changes must be addressed to merge this pull request.

Assignees

@dd-dominic dd-dominic

Labels
agent/review-requested docs/review-requested ecosystems/requested-changes kind/technology-partner product/requested-changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nikhil-singh-01 @bgoldberg122 @Kyle-Neale @dd-dominic