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 namespace to jvm metric attributes #20

Merged
merged 6 commits into from
Aug 14, 2023

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented May 15, 2023

@zeitlinger zeitlinger marked this pull request as ready for review May 16, 2023 09:55
@zeitlinger zeitlinger requested review from a team as code owners May 16, 2023 09:55
@jsuereth jsuereth mentioned this pull request May 22, 2023
@trask
Copy link
Member

trask commented May 26, 2023

From JVM runtime metrics stability WG, I think we had agreed on these attribute names if the outcome of #51 is to namespace metric attributes

schemas/1.21.0 Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
schemas/1.21.0 Outdated Show resolved Hide resolved
schemas/1.21.0 Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@zeitlinger zeitlinger force-pushed the jvm_metric_attributes branch 2 times, most recently from 0a1eb2e to 648f5c8 Compare June 27, 2023 10:30
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.

I posted a comment expressing a preference for a difference strategy, but like this as well if folks are aligned that gc. / memory. / thread. / buffer. are generic enough concepts to not place in a JVM namespace.

schemas/1.21.0 Outdated Show resolved Hide resolved
schemas/1.21.0 Outdated Show resolved Hide resolved
@joaopgrassi
Copy link
Member

I posted a #61 (comment) expressing a preference for a difference strategy, but like this as well if folks are aligned that gc. / memory. / thread. / buffer. are generic enough concepts to not place in a JVM namespace.

I also have some concerns with the broad gc, memory, thread prefixes. If later we want to document attributes for other GCs then they will collide with these. If they are broad enough, then it probably makes more sense to move them to a separate file, like done for http in http-common.yaml. You can see there the prefix is just http for the attribute_group.

@zeitlinger
Copy link
Member Author

@joaopgrassi fixed the obvious things

@zeitlinger
Copy link
Member Author

@joaopgrassi we decided to qualify all metrics with "jvm." except for thread.daemon - everything should be addressed now

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
semantic_conventions/metrics/thread.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Primarily, the schema file should NOT be in the schemas directory, we only create that one when cutting a release.

However, additionally to this attribute rename, we should be renaming the JVM metric namespace as a whole to be jvm.* based on a lot of discussions.

Particularly - We want namespacing to be "reasonably unique" and minimal. process.runtime just doesn't make any sense for the JVM. The JVM is not bound to one process, depending on which architecture it's run on.

schemas/1.22.0 Outdated Show resolved Hide resolved
schema-next.yaml Outdated Show resolved Hide resolved
schema-next.yaml Outdated Show resolved Hide resolved
@jsuereth
Copy link
Contributor

jsuereth commented Aug 2, 2023

While I still think we should entertain changing the metrics runtime.process.jvm. => jvm., I think this makes good incremental progress. Still need to address Tigran's comments on the schema file.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 2, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@trask
Copy link
Member

trask commented Aug 2, 2023

Blocking until the confusion about using top-level namespaces is clarified.

@tigrannajaryan are you ok with unblocking this PR and moving the discussion to #227? or would you like to get resolution on #227 before merging this?

@trask
Copy link
Member

trask commented Aug 2, 2023

/easycla

@joaopgrassi joaopgrassi dismissed their stale review August 8, 2023 07:34

It's been a while since I reviewed this, given I think things changed, I will look at it again

@joaopgrassi
Copy link
Member

joaopgrassi commented Aug 10, 2023

Depending which one merges first (#241) one of them will have to adapt either the metric names or the attribute names in the changelog/schema file

@trask
Copy link
Member

trask commented Aug 13, 2023

Depending which one merges first (#241) one of them will have to adapt either the metric names or the attribute names in the changelog/schema file

#241 got merged first, I have resolved conflicts in this PR

CHANGELOG.md Outdated Show resolved Hide resolved
@trask
Copy link
Member

trask commented Aug 14, 2023

However, for the attribute names we suggest jvm.* as the namespace. Why this and why not process.runtime.jvm.*? From what I understand it is because jvm.* is shorter. But is it unique enough to ensure there are no clashes? If it is then why not use it also for metric names, e.g. jvm.memory.usage?

@tigrannajaryan we have shortened the metric names also now from process.runtime.jvm.* to jvm.* (#241)

if this resolves your concern, can you unblock? thx

CHANGELOG.md Outdated Show resolved Hide resolved
@reyang reyang closed this Aug 14, 2023
@reyang reyang reopened this Aug 14, 2023
@reyang reyang merged commit 2927d58 into open-telemetry:main Aug 14, 2023
16 of 17 checks passed
ChrsMark pushed a commit to ChrsMark/semantic-conventions that referenced this pull request Aug 16, 2023
@zeitlinger zeitlinger deleted the jvm_metric_attributes branch August 21, 2023 12:06
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.

Add namespaces to JVM metric attributes?
9 participants