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

Jetty Client instrumentation with Observation API #3416

Merged
merged 5 commits into from
Feb 15, 2023
Merged

Jetty Client instrumentation with Observation API #3416

merged 5 commits into from
Feb 15, 2023

Conversation

shakuzen
Copy link
Member

@shakuzen shakuzen commented Sep 16, 2022 •
edited
Loading

Allow configuring an ObservationRegistry so that the JettyClientMetrics can instrument with Observation API in addition to its previously existing functionality.

TODO:

  • Tests with Observation
  • ObservationDocumentation

@shakuzen shakuzen marked this pull request as ready for review September 21, 2022 04:14
@shakuzen
Copy link
Member Author

Currently, we don't have the URI keyvalue available on start because the mapping function is based on the Result which isn't available on start. I'm not sure how best to fix this.

@shakuzen
Copy link
Member Author

In order to solve the problem with not being able to tag URI on start, I introduced a uri mapping function that takes a Request and Result. The Result will be null on start, but the Request should be available. Without this, the DefaultMeterObservationHandler could at best make a LongTaskTimer that didn't tag uri or tagged it with a value of UNKNOWN. Either option made the LTT far less useful as there was no breakdown whatsoever by uri.

I tried to be careful about introducing the new mapping function in a way that is backwards compatible and guide users with deprecations. The previous tests in JettyClientMetricsTest are passing without changing anything there.

@shakuzen
WIP Jetty Client instrumentation with Observation
6f70df6 
Allow configuring an ObservationRegistry so that the JettyClientMetrics can instrument with Observation API in addition to its previously existing functionality.
@shakuzen
Fix formatting
6e152b4
@shakuzen
TCK with observation
5539870 
Needed to update JettyClientKeyValues to handle the case when the result was null, which will always happen when getting the low cardinality tags for the LongTaskTimer at the start of timing. For now, I've updated to return the full set of tags with an UNKNOWN value for things that can't be derived with a null result.
@shakuzen
Use Request for URI tagging
7569065 
Adds configuring a uri mapping function that takes a request and result, which is needed for tagging URI on start as opposed to only on stop.
This intends to be backwards compatible with deprecations pointing users at the new API.
@shakuzen
Add ObservationDocumentation implementation and polish
d281f37 
Updates the since version in JavaDocs and adds the ObservationDocumentation implementation. It is still not used by ObservationOrTimerCompatibleInstrumentation but that can be updated separately.
@shakuzen shakuzen changed the title WIP Jetty Client instrumentation with Observation Jetty Client instrumentation with Observation API Feb 8, 2023
@shakuzen shakuzen added enhancement A general enhancement module: micrometer-core An issue that is related to our core module labels Feb 8, 2023
@shakuzen shakuzen added this to the 1.11 backlog milestone Feb 8, 2023
@shakuzen
Copy link
Member Author

shakuzen commented Feb 8, 2023

I noticed when working on this that ObservationOrTimerCompatibleInstrumentation doesn't take an ObservationDocumentation. But we can consider whether anything needs to be done about that separately from this pull request, I think.

This is ready for review now.

@shakuzen shakuzen modified the milestones: 1.11 backlog, 1.11.0-M2 Feb 15, 2023
@shakuzen shakuzen merged commit 54f89f4 into micrometer-metrics:main Feb 15, 2023
@shakuzen shakuzen deleted the jetty-client-obs branch February 15, 2023 06:47
izeye added a commit to izeye/micrometer that referenced this pull request Feb 21, 2023
@izeye
Polish "Jetty Client instrumentation with Observation API"
44b8537 
See micrometer-metricsgh-3416
@izeye izeye mentioned this pull request Feb 21, 2023
Merged
shakuzen pushed a commit that referenced this pull request Feb 21, 2023
@izeye
Polish "Jetty Client instrumentation with Observation API" (#3659)
4884ec8 
See gh-3416
@shakuzen shakuzen mentioned this pull request Feb 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift sonatype-lift left review comments

@marcingrzejszczak marcingrzejszczak marcingrzejszczak approved these changes

@jonatan-ivanov jonatan-ivanov Awaiting requested review from jonatan-ivanov

Assignees
No one assigned
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Projects
None yet
Milestone
1.11.0-M2
Development

Successfully merging this pull request may close these issues.
2 participants
@shakuzen @marcingrzejszczak