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

Support process.runtime.* resource attributes #1908

Closed
Oberon00 opened this issue Oct 28, 2020 · 8 comments · Fixed by #2143
Closed

Support process.runtime.* resource attributes #1908

Oberon00 opened this issue Oct 28, 2020 · 8 comments · Fixed by #2143
Labels
Feature Request Suggest an idea for this project

Comments

@Oberon00
Copy link
Member

Oberon00 commented Oct 28, 2020

There should be support for the process.runtime.* resource attributes, which should contain information about the monitored JVM.

Spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/process.md#java-runtimes

Note that I made a spec PR to split process.runtime from process, so that these resources can be reported independently: open-telemetry/opentelemetry-specification#1137
Since these values are trivial to gather (just System.getProperty), it might make sense to always report them out of the box along with the telemetry SDK.

Or instead it may make sense to add them to a provider separate from the current process provider.

@Oberon00 Oberon00 added the Feature Request Suggest an idea for this project label Oct 28, 2020
@Oberon00
Copy link
Member Author

Oberon00 commented Oct 28, 2020

A provider-based implementation could look like this:

public class ProcessRuntimeResourceProvider extends ResourceProvider {

  @Override
  protected Attributes getAttributes() {
    return Attributes.of(
        PROCESS_RUNTIME_NAME,
        System.getProperty("java.runtime.name"),
        PROCESS_RUNTIME_VERSION,
        System.getProperty("java.runtime.version"),
        PROCESS_RUNTIME_DESCRIPTION,
        System.getProperty("java.vm.vendor") + ' ' + System.getProperty("java.vm.version"));
  }
}

@jkwatson
Copy link
Contributor

My opinion: we should report them by default.

@Oberon00
Copy link
Member Author

Oberon00 commented Nov 17, 2020

@anuraaga From some looking around, it seems that java.vm.name is usually more interesting and specific than java.runtime.name. Do you think the spec should be changed? E.g. OpenJDK-based JVMs seem to have java.runtime.name=OpenJDK Runtime Environment but report something at least a bit more specific in java.vm.name, e.g. on Windows I can see for the same Adopt OpenJDK 15 version java.vm.name=OpenJDK 64-Bit Server VM for 64 bit but OpenJDK Client VM for 32 bit.

@anuraaga
Copy link
Contributor

Here's my gist for reference

https://gist.github.com/anuraaga/c79219a7e4401515c3de998f9d077be0

I think one thing that lead me to the current properties is I think this allowed OpenJ9 to model well. It's still OpenJDK, but with different VM. So keeping runtime.name constant with the other OpenJDK I think works well.

@anuraaga
Copy link
Contributor

Admittedly, I thought client VM had already been deleted by now... But I'd also assume any use case for telemetry would force server mode on anyways, which I guess requires a flag on 32-bit?

@Oberon00
Copy link
Member Author

Indeed, if I use the -server option, I get java.vm.name = OpenJDK Server VM (note the absence of 64-Bit in the name).

@Oberon00
Copy link
Member Author

Oberon00 commented Nov 17, 2020

For J9, what would be the problem with using java.vm.name = Eclipse OpenJ9 VM as process.runtime.name?

(BTW, I think we need some kind of process.runtime.kind in the spec that is only python, jvm, net without too much specifics OK this should be covered by telemetry.sdk.language, although that's a bit unintuitive EDIT2: open-telemetry/opentelemetry-specification#1232 adds a note)

@Oberon00
Copy link
Member Author

@anuraaga I created open-telemetry/opentelemetry-specification#1242 to add the java.vm.name to process.runtime.description. Please take a look.

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

Successfully merging a pull request may close this issue.

3 participants