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.

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

add metric annotation instrumentation #11354

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

Duncan-tree-zhou
Copy link

@Duncan-tree-zhou Duncan-tree-zhou commented May 14, 2024

refer to #7030
add metric annotation instrumentation

@Duncan-tree-zhou Duncan-tree-zhou requested a review from a team May 14, 2024 15:45
@steverao
Copy link
Contributor

There are some CI failures, firstly you can solve them by referring to https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/running-tests.md#troubleshooting-ci-test-failures

@Duncan-tree-zhou Duncan-tree-zhou marked this pull request as draft May 15, 2024 15:00
@Duncan-tree-zhou Duncan-tree-zhou force-pushed the add_metric_annotation_instrument branch 2 times, most recently from f1797c8 to f7dd6b9 Compare May 20, 2024 13:27
@Duncan-tree-zhou
Copy link
Author

Hi @steverao, I found that there are lots of fails "to connect to localhost/127.0.0.1:4318". So I want to rerun the task, but I can't see the rerun button. Is it disabled for contributors?

@steverao
Copy link
Contributor

Is it disabled for contributors?

Yes, you can fix the problems firstly and push relevant commits. It will trigger to rerun the CI tasks.

@Duncan-tree-zhou
Copy link
Author

Duncan-tree-zhou commented May 22, 2024

I mean I guest the failure is caused by the CI run time environment crush..... if it's able to rerun a task, It might save some time....

@Duncan-tree-zhou Duncan-tree-zhou force-pushed the add_metric_annotation_instrument branch 2 times, most recently from 815bbfd to 95f0f0d Compare May 24, 2024 14:29
@github-actions github-actions bot requested a review from theletterf May 29, 2024 16:36
@Duncan-tree-zhou Duncan-tree-zhou force-pushed the add_metric_annotation_instrument branch from f9657ec to 5ab650b Compare May 30, 2024 14:13
@Duncan-tree-zhou Duncan-tree-zhou marked this pull request as ready for review May 30, 2024 14:58
@Duncan-tree-zhou
Copy link
Author

Duncan-tree-zhou commented May 30, 2024

I turn it ready for reviewing, and get some questions for discuss:

  1. the module naming of the @Counted and @Timed instrumentation.
    • I avoid using the opentelemetry-instrumentation-annotations prefix because it's already been used for @Withspan. I am not sure if using opentelemetry-instrumentation-annotations-coutned would cause some problem somewhere.
    • I have no idea if there is a better module naming rule for metrics annotations instrumentations.
  2. I am not sure if we should put the instance of meter, LongCounter and DoubleHistogram in another module where can be loaded without javaagent?
  3. I am doubted about allowing the return value to be an attributes because most of time the return value is unpredictable. would it be possible to design some static api to allow user to post the attributes to the metrics via threadlocal?

@Duncan-tree-zhou Duncan-tree-zhou force-pushed the add_metric_annotation_instrument branch 5 times, most recently from 5024350 to 54866bf Compare June 9, 2024 04:27
@Duncan-tree-zhou Duncan-tree-zhou force-pushed the add_metric_annotation_instrument branch from 54866bf to 21cbf96 Compare June 9, 2024 04:30
@trask trask added this to the v2.9.0 milestone Sep 13, 2024
@trask trask modified the milestones: v2.9.0, v2.10.0 Oct 17, 2024
@trask trask modified the milestones: v2.10.0, v2.11.0 Nov 13, 2024
@trask trask modified the milestones: v2.11.0, v2.12.0 Dec 23, 2024
@Duncan-tree-zhou Duncan-tree-zhou requested a review from a team as a code owner January 7, 2025 13:14
@trask trask modified the milestones: v2.12.0, v2.13.0 Jan 17, 2025
@trask trask removed this from the v2.13.0 milestone Feb 13, 2025
@Duncan-tree-zhou Duncan-tree-zhou force-pushed the add_metric_annotation_instrument branch from 27df0c9 to 7c1b5f0 Compare March 5, 2025 14:09
@Duncan-tree-zhou Duncan-tree-zhou force-pushed the add_metric_annotation_instrument branch from 7c1b5f0 to bb4cb98 Compare March 5, 2025 14:33
import javax.annotation.Nullable;

/** Callback that is called when async computation completes. */
public interface AsyncOperationEndHandler<REQUEST, RESPONSE> {
Copy link

Choose a reason for hiding this comment

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

I get the name, but I think it's too specific and long.
What else can it handle apart from ending a span?

Copy link
Author

@Duncan-tree-zhou Duncan-tree-zhou Mar 5, 2025

Choose a reason for hiding this comment

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

I think it needs to handle the end of the timer and record the duration to the histogram metrics recorder.

Copy link

@brunobat brunobat Mar 6, 2025

Choose a reason for hiding this comment

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

This is effectively an Instrumentation Callback... Why not call it that?
Something like InstrumenterCallback with an end() method...
I can imagine this also being useful to add attributes if later we add an addAttribute() method.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @brunobat, I go through the code. The class Instrumentation is coupled with the implementation of the span's operations(here) and it's widely used. It's going far from the guide's suggestion(here). I think to create a new set of abstract operation for metrics instrumentation and update the guide would be an option. @laurit gave a good example of it. And what's your opinion about it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants