Skip to content

Live Metrics Filtering Part 4: Filtering For Metrics Charts#43323

Merged
harsimar merged 18 commits into
mainfrom
harskaur/projection
Dec 19, 2024
Merged

Live Metrics Filtering Part 4: Filtering For Metrics Charts#43323
harsimar merged 18 commits into
mainfrom
harskaur/projection

Conversation

@harsimar

@harsimar harsimar commented Dec 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Description

  • Added DerivedMetricProjections.java to handle calculation of derived metrics
  • Using this class in the QuickPulseDataCollector to create/calculate derived metrics with each incoming telemetry item
  • Derived metrics now being included in post request body
  • Implement applyMetricFilters() in QuickPulseDataCollector such that derived metrics are created/calculated based on filtering rules defined in metrics charts filtering configuration.

Above changes enable e2e filtering for metrics charts.

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 Dec 11, 2024
jeanbisutti
jeanbisutti reviewed Dec 11, 2024
View reviewed changes
Comment thread ...pentelemetry/autoconfigure/implementation/quickpulse/filtering/DerivedMetricProjections.java Outdated
Comment thread ...pentelemetry/autoconfigure/implementation/quickpulse/filtering/DerivedMetricProjections.java Outdated
@azure-sdk

azure-sdk commented Dec 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

com.azure:azure-monitor-opentelemetry-autoconfigure

jeanbisutti
jeanbisutti reviewed Dec 16, 2024
View reviewed changes

@jeanbisutti jeanbisutti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps in another PR it may be worth to add logs at the TRACE or DEBUG level to understand filtering and derived metrics behaviors from a user application.

Comment thread ...etry/autoconfigure/implementation/quickpulse/filteringTest/DerivedMetricProjectionsTest.java Outdated
Comment thread ...etry/autoconfigure/implementation/quickpulse/filteringTest/DerivedMetricProjectionsTest.java Outdated
Comment thread ...etry/autoconfigure/implementation/quickpulse/filteringTest/DerivedMetricProjectionsTest.java Outdated
@harsimar

harsimar commented Dec 16, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@jeanbisutti @trask
Regarding concurrency, I finally got around to logging the thread name of different components (as an experiment). In summary, a QuickPulseCoordinator thread is responsible for calling the constructors of DerivedMetricProjections/Aggregation and reading final metric values, while java agent threads (BatchLog/SpanProcessor worker threads) are responsible for updating the values inside each DerivedMetricAggregation via quickpulse.add(), which later calls QuickPulseDataCollector.add() that processes each telemetry item for live metrics filtering. I do not see the same DerivedMetricAggregation instance getting its fields written to at exactly the same time due to synchronization of writes.

Seeing this & noting that the writes/reads are all inside synchronized blocks, I am tempted to change count/aggregation inside of DerivedMetricAggregation class be just long and double, as opposed to AtomicLong/AtomicDouble. What do you think about this?

@harsimar harsimar marked this pull request as ready for review December 16, 2024 22:43
jeanbisutti
jeanbisutti reviewed Dec 19, 2024
View reviewed changes
Comment thread ...onitor/opentelemetry/autoconfigure/implementation/quickpulse/filtering/CustomDimensions.java
Comment thread ...pentelemetry/autoconfigure/implementation/quickpulse/filtering/DerivedMetricAggregation.java
Comment thread ...pentelemetry/autoconfigure/implementation/quickpulse/filtering/DerivedMetricAggregation.java
Comment thread ...pentelemetry/autoconfigure/implementation/quickpulse/filtering/DerivedMetricProjections.java Outdated
Comment thread ...pentelemetry/autoconfigure/implementation/quickpulse/filtering/DerivedMetricProjections.java
@harsimar

harsimar commented Dec 19, 2024

Copy link
Copy Markdown
Member Author

/check-enforcer override

@harsimar harsimar merged commit c5e74a8 into main Dec 19, 2024
@harsimar harsimar deleted the harskaur/projection branch December 19, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trask trask trask left review comments

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

@heyams heyams Awaiting requested review from heyams

+1 more reviewer

@jeanbisutti jeanbisutti jeanbisutti approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

OpenTelemetry OpenTelemetry instrumentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@harsimar @azure-sdk @trask @jeanbisutti