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

Micrometer Observation support #1879

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Micrometer Observation support #1879

wants to merge 1 commit into from

Conversation

marcingrzejszczak
Copy link

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - Micrometer Observation Support #1868
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1868 ☕️

If you write sample code, please follow the samples format.

Discussion points

This PR extracts the logic that the OpenCensus instrumentation used to instrument an outbound request into an interface that other implementations (Micrometer Observation, OpenTelemetry etc.) could implement. For backward compatibility I left the OpenCensus one as the one that is always being set (you can override it).

Problems

  • There's a lot of methods in the extracted interface - maybe we could simplify it?
  • Micrometer Observation uses JDK 8 (I have added it as a separate module to show that instrumentation can happen with the new interface)
  • I haven't added Javadocs nor docs
  • I haven't added customization options (via Micrometer Observation Conventions)

Results of running the tests are

.Metrics

Sep 15, 2023 2:35:58 PM io.micrometer.tracing.test.SampleTestRunner printMetrics
INFO: Gathered the following metrics
	Meter with name <http.client.duration.active> and type <LONG_TASK_TIMER> has the following measurements 
		<[Measurement{statistic='ACTIVE_TASKS', value=0.0}, Measurement{statistic='DURATION', value=0.0}]> 
		and has the following tags <[]>
	Meter with name <http.client.duration> and type <TIMER> has the following measurements 
		<[Measurement{statistic='COUNT', value=1.0}, Measurement{statistic='TOTAL_TIME', value=0.001958896}, Measurement{statistic='MAX', value=0.001958896}]> 
		and has the following tags <[tag(error=none), tag(http.status_code=200)]>
	Meter with name <zipkin_brave.active> and type <LONG_TASK_TIMER> has the following measurements 
		<[Measurement{statistic='ACTIVE_TASKS', value=1.0}, Measurement{statistic='DURATION', value=0.103744693}]> 
		and has the following tags <[]>

.Spans - Brave
image

.Spans - OpenTelemetry
image

@google-cla
Copy link

google-cla bot commented Sep 15, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Sep 15, 2023 •
edited
Loading

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Sep 15, 2023
@marcingrzejszczak marcingrzejszczak mentioned this pull request Sep 15, 2023
Open
@blakeli0
Copy link

blakeli0 commented Sep 26, 2023 •
edited
Loading

Thanks @marcingrzejszczak for opening this PR! However, this project still has to support Java 7 as mentioned in the README, it would be hard for us to accept this because Micrometer Observation uses JDK 8. Making it a separate module could work but we would have to build proper testing infra for it, which is not planned in the near future.

@marcingrzejszczak
Copy link
Author

Hey, thanks for the feedback. You've mentioned that you're doing a POC with OTel googleapis/sdk-platform-java#1807 and it also requires JDK8. So how are you going to go with the infra as far as that PR is concerned? I can then do the same with this one.

@marcingrzejszczak marcingrzejszczak mentioned this pull request Sep 27, 2023
Open
@blakeli0
Copy link

Hey, thanks for the feedback. You've mentioned that you're doing a POC with OTel googleapis/sdk-platform-java#1807 and it also requires JDK8. So how are you going to go with the infra as far as that PR is concerned? I can then do the same with this one.

This repo is a dependency of Google API Client Libraries which still supports Java 7, sdk-platform-java contains a few dependencies of Google Cloud Client Libraries, which is the newer and recommended way to access Cloud APIs, and requires Java 8. I know it's confusing and we are trying to make it clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Micrometer Observation Support
2 participants
@marcingrzejszczak @blakeli0