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

Update java metric instrumentation docs #4165

Merged

Conversation

jaydeluca
Copy link
Member

@jaydeluca jaydeluca commented Mar 15, 2024

Closes #4123

I tried to mirror the javascript docs as closely as possible. Let me know if anything should be revisited or changed.

@jaydeluca jaydeluca requested review from a team March 15, 2024 00:50
@jeanbisutti
Copy link
Member

cc @jack-berg

Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Some style observations added.

content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
@cartermp cartermp added the sig-approval-missing Co-owning SIG didn't provide an approval label Mar 17, 2024
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

I read through this and think that it's a considerable improvement. Didn't find anything worth mentioning to improve/change. Thanks!

@cartermp cartermp removed the sig-approval-missing Co-owning SIG didn't provide an approval label Mar 20, 2024
@cartermp
Copy link
Contributor

/fix:all

Copy link
Contributor

You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/8361878119

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Pretty nice improvement! I left a few comments but I'm generally supportive of this change.

content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
### Using Observable (Async) Counters

Observable counters can be used to measure an additive, non-negative,
monotonically increasing value.
Copy link
Member

Choose a reason for hiding this comment

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

Its important to note that observable counters observe cumulative values which have been aggregated elsewhere. I.e. you don't asynchronously observe the increment you would record to a sync counter - you asynchronously observe the cumulative state of a bunch of increments. Same adivce applies to async up down counter.

Also, the example of monitoring the cache size with an async counter is wrong because caches can reduce in size, violating the monotonic requirement. Not sure what example you should use instead - one that comes to mind is the cumulative number of total rolls (but this isn't great because it duplicates data present in the dice-lib.rolls histogram), or the number of classes loaded by the JVM.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made some changes to reflect the distinction between the observables, and I think i have more realistic examples for the metric examples now but let me know if you think they still don't match

content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Noticed a couple more minor things I missed the first time around, but looks good. Thanks @jaydeluca!

content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/java/instrumentation.md Outdated Show resolved Hide resolved
@cartermp cartermp merged commit f703e98 into open-telemetry:main Mar 23, 2024
14 checks passed
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.

Java Metrics Documentation could use an update
6 participants