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

[Azure-Monitor-Opentelemetry-Exporter] Use the Live Metrics Swagger Definition & Fix request backoff behavior #42769

Merged
merged 18 commits into from
Nov 18, 2024
Merged

[Azure-Monitor-Opentelemetry-Exporter] Use the Live Metrics Swagger Definition & Fix request backoff behavior #42769

merged 18 commits into from
Nov 18, 2024

Conversation

harsimar
Copy link
Member

@harsimar harsimar commented Oct 31, 2024 •
edited
Loading

Description

This PR includes the following changes:

  • Include the classes that are a part of the live metrics swagger definition
  • Use some of those classes when making ping/post requests & building the monitoring data points to send to quickpulse
  • Remove old classes not used anymore as a result of incorporating the swagger definition
  • Modifications to tests so that the tests adhere to the swagger definition. Most testing was done manually. In a future PR I may add more automated tests.
  • There are some previously disabled tests that currently don't pass. I will fix this later.
  • Minor fixes to lastTransmissionTime logic, such that api calls adhere to backoff behavior described here in all edge cases: https://github.com/aep-health-and-standards/Telemetry-Collection-Spec/blob/main/ApplicationInsights/livemetrics.md#timings

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • [] Pull request includes test coverage for the included changes.

@github-actions github-actions bot added the OpenTelemetry OpenTelemetry instrumentation label Oct 31, 2024
@harsimar harsimar marked this pull request as ready for review November 4, 2024 23:43
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

ramthi
ramthi reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@ramthi ramthi left a comment

Choose a reason for hiding this comment

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

LGTM, pl. merge when someone from Java team has reviewed and approved.

@jeanbisutti
Copy link
Contributor

Merged in the OpenTelemetry autoconfigure module: #42864

@harsimar
Copy link
Member Author

Merged in the OpenTelemetry autoconfigure module: #42864

Hey @jeanbisutti - just to clarify, pr #42864 moved swagger changes from exporter library to the autoconfigure library within this PR branch. However, the overall swagger changes are not merged to main. Please review the swagger changes when you have the chance, thanks!

jeanbisutti
jeanbisutti reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@jeanbisutti jeanbisutti left a comment

Choose a reason for hiding this comment

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

It would be great to test with the Application Insights smoke tests.

...nitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseNetworkHelperTest.java Show resolved Hide resolved
...ure/monitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseCoordinator.java Outdated Show resolved Hide resolved
...ure/monitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseCoordinator.java Show resolved Hide resolved
...ure/monitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseCoordinator.java Outdated Show resolved Hide resolved
...ure/monitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseCoordinator.java Show resolved Hide resolved
...e/monitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseDataCollector.java Show resolved Hide resolved
...ure/monitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseDataFetcher.java Show resolved Hide resolved
...onitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseDataFetcherTests.java Show resolved Hide resolved
@jeanbisutti
Copy link
Contributor

Merged in the OpenTelemetry autoconfigure module: #42864

Hey @jeanbisutti - just to clarify, pr #42864 moved swagger changes from exporter library to the autoconfigure library within this PR branch. However, the overall swagger changes are not merged to main. Please review the swagger changes when you have the chance, thanks!

I have not seen this notif, and I thought that the other PR was merged on main. Next time, don't hesitate to ping me with a Github link at MS if I don't reply the next day.

@trask
Copy link
Member

trask commented Nov 14, 2024

It would be great to test with the Application Insights smoke tests.

are you suggesting to test this PR with the existing java agent smoke tests (sadly we don't have any for live metrics), or are you suggesting that we write java agent smoke tests (@harsimar and I have been discussing we want to do that, but will come later on in this project, not part of this PR)

@jeanbisutti
Copy link
Contributor

jeanbisutti commented Nov 14, 2024 •
edited
Loading

he existing java agent smoke tests (sadly we don't have any for live metrics), or are you suggesting that we write java agent smoke tests (@harsimar and I have been discussing we want to d

I have forgotten that there are not live metric tests for the Java agent.

I would suggest:

  1. Merge this PR, or a part of this PR, if it seems reasonable
  2. Write an end to end test in the agent
  3. Execute the agent end to end test with the new code locally, and fix an issue if there is one

jeanbisutti
jeanbisutti approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@jeanbisutti jeanbisutti left a comment

Choose a reason for hiding this comment

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

LGTM

More testing after would be great (#42769 (comment))

@harsimar
Copy link
Member Author

/check-enforcer override

@harsimar harsimar merged commit 41e8fa2 into main Nov 18, 2024
17 of 19 checks passed
@harsimar harsimar deleted the harskaur/swaggerQP branch November 18, 2024 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramthi ramthi ramthi left review comments

@trask trask trask left review comments

@jeanbisutti jeanbisutti jeanbisutti approved these changes

@heyams heyams Awaiting requested review from heyams heyams is a code owner

Assignees
No one assigned
Labels
OpenTelemetry OpenTelemetry instrumentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@harsimar @azure-sdk @jeanbisutti @trask @ramthi