open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.69k stars 884 forks source link

Remove process.runtime semconv for browser resource #2806

Open scheler opened 1 year ago

scheler commented 1 year ago

What are you trying to achieve?

I am trying to remove the use of process.runtime.* attributes for browser resource. In the Client Instrumentation SIG we agreed on using only browser. namespace in the browser resource for identification purpose. The process.runtime.* predates the browser namespace and is added in BrowserDetector.ts. Checking with few members in the SIG, it's not clear if anyone is using this detector currently.

We discussed this in the 9/14 JS SIG and here - the plan is to implement a new detector for browser in a separate package and mark the current BrowserDetector in opentelemetry-resources package as deprecated.

So for the purpose of marking this detector as deprecated, we want to remove references to Web Browser from JavaScript Runtimes in process.md first. Note that the process semconv is still in Experimental state.

I will submit a PR for the removal, but creating this issue first to see if there are any concerns from the spec members.

@legendecas @martinkuba @MSNev

legendecas commented 1 year ago

I support replacing the process.runtime.* for web browsers with the new browser.* attributes. It would be more meaningful to have detailed browser attributes than the plain UA strings. Deprecating the legacy attributes and introducing a new detector helps us avoid making breaking changes as the legacy detector has been shipped since the beginning.

jsuereth commented 1 year ago

I'm in favor of this change.

Oberon00 commented 1 year ago

I oppose this change.

Why? Because I think there should be a single resource attribute that tells you from which kind of process/runtime any telemetry is emitted from. Of course, adding additional information in a browser.* namespace is totally fine, but having basic process.runtime.* information there is valuable as it allows generic handling of resource information.

With this change you are requiring handling such as

if (resourceAttributes.contains("browser.whatever") {
 // Handle process type identification based on browser
} else {
  // Handle process type identifaction based on process.runtime
}

Instead I'd like to be able to do handling that I only need to look into browser.* if I am actually interested in browser-specific attributes.

Maybe the current definitions for process.runtime for browser are bad, then they should be changed. But I don't think we should start making this "deprecated" for any technology.

scheler commented 1 year ago

Why? Because I think there should be a single resource attribute that tells you from which kind of process/runtime any telemetry is emitted from

The attribute telemetry.sdk.language represents the general kind of runtime and should be used first, and process.runtime.* can be used for further identification within each. For browsers there is only one runtime identity.

Oberon00 commented 1 year ago

I see telemetry.sdk.language has been repurposed to also distinguish nodejs and webjs even though both use javascript. I did not know that. Still, that doesn't really change my argument. If you want to show a textual description of the runtime, you could use process.runtime.description || process.runtime.name + process.runtime.version everywhere. Why not also for browser-based runtimes?

scheler commented 1 year ago

I don't know if any vendor wants to show a textual description of the runtime for browser apps. We had discussed this in the Client Instrumentation SIG and this didn't come up as a requirement.