search

open-telemetry / opentelemetry-js-contrib

OpenTelemetry instrumentation for JavaScript modules
https://opentelemetry.io
Apache License 2.0
637 stars 476 forks source link

Document instrumentation semantic conventions #1778

Open dyladan opened 7 months ago

dyladan commented 7 months ago

Many packages in OTel JS Contrib export telemetry with out of date semantic conventions. Unfortunately, we have not clearly documented the version of semantic conventions implemented or which attributes are exported for instrumentations. For each package, we need to document the telemetry exported and if possible update it to the latest semantic conventions following the following template:

This package implements the <xxxx> semantic convention version X.Y: <link to semantic convention>

Attributes collected:

| Attribute | Short Description | Notes |
| --------- | ----------------- | ----- |
|           |                   |       |

Please limit PRs to a single package in order to make them quickly and easily reviewable. If you are working on one of the below packages, please comment so others know the issue is being worked on.

serverless-mom commented 7 months ago

Going to take a look at instrumentation-fs

pkanal commented 7 months ago

👋🏽 If you're taking a look at this issue for contribfest, please comment which package you're specifically working on!

maig123 commented 7 months ago

will take a shot at instrumentation-amqlibs

JamieDanielson commented 5 months ago

@dyladan I don't think we want the "latest" semantic conventions just yet, right, because of the breaking changes in HTTP attributes. I think we'd want to ensure we have the latest before the break (so v1.20), and then an issue/PR to add the OTEL_SEMCONV_STABILITY_OPT_IN environment variable to allow for opting into the new attributes and/or dual attributes?

dyladan commented 5 months ago

Yes I think so

JamieDanielson commented 5 months ago

Here is my understanding of the current state:

Each package uses either @opentelemetry/semantic-conventions, of which our latest version is 1.20 based on current stable otel-js release, or hard-codes attributes (haven't confirmed if this hard-coding exists or not, just speculating).

Our package @opentelemetry/semantic-conventions uses attributes generated from otel-js scripts/semconv, which currently uses spec version 1.7.0.

This means any packages using our @opentelemetry/semantic-conventions package are using semantic convention version 1.7, but we want to be on semantic convention version 1.20. As an example, here is http.user_agent in our package which does not match the newly named user_agent.original that was introduced in v1.19.0.

I guess that is what is referenced in https://github.com/open-telemetry/opentelemetry-js/issues/4235, which I'm also now realizing has a somewhat recent update to it. These two things will be very much in line. One possible approach for the immediate term would be to simply document all of these packages in this issue to say they emit semantic convention version 1.7, and if any semantic convention attribute is hard-coded in a package it should be replaced with the constant. Then separately we address https://github.com/open-telemetry/opentelemetry-js/issues/4235 to update semantic conventions. Eventually we will then want to upgrade to 1.23+ with the aforementioned variables to ease pain on end users, but it seems we have some steps to take first.

What do you think?

JamieDanielson commented 5 months ago

I opened a PR for Express to help get this started and to have as a guide for the others. Can you take a look and confirm whether this is what we're trying to do, or if I'm missing more on this?

pichlermarc commented 4 months ago

I opened a PR for Express to help get this started and to have as a guide for the others. Can you take a look and confirm whether this is what we're trying to do, or if I'm missing more on this?

I think it does exactly the right thing, thanks for taking care of that. :+1:

JamieDanielson commented 3 months ago

For others that come across this and want to update for another package, this is the structure added to the Express instrumentation package, which can be used as a guide for the other packages:

## Semantic Conventions

This package uses `@opentelemetry/semantic-conventions` version `1.0+`, which implements Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md)

Attributes collected:

| Attribute    | Short Description                  | Notes             |
| ------------ | ---------------------------------- | ----------------- |
| `http.route` | The matched route (path template). | Key: `HTTP_ROUTE` |
JamieDanielson commented 3 months ago

Updating instrumentation-document-load package https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2039 to document semantic conventions

trentm commented 2 months ago

@JamieDanielson (/cc @pichlermarc @maryliag @david-luna): About the Attributes collected: table.

Is there much value in the Notes column with the Key: SOME_VAR values?

I think these property names on the semconv package are internal implementation details that the user of these packages doesn't need to know about. When a user uses @opentelemetry/instrumentation-express, this table is helpful to know that they should expect the http.route attribute on spans. However, they have no need to know about:

import { SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions';

Having that column is more just a (light) maintenance burden, and distraction.

Thoughts?

I realize I'm asking a little late. There are a lot of PRs for this and https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2025 that are adding this table to READMEs.

maryliag commented 2 months ago

I agree. I believe the columns for Attribute and Descriptions are important, this is why I was creating several PRs to add them, but don't see much value on adding the key column, since it doesn't affect the usage and the user shouldn't care about it. Also, as you mention adds overhead to maintain.

JamieDanielson commented 2 months ago

Yeah that's a fair point. I can see it being plenty useful to keep just the attribute key and short description, and omitting the internal key field. So instead of previous it could just be (after the exported string change, 1.0+ becomes 1.22+)

## Semantic Conventions

This package uses `@opentelemetry/semantic-conventions` version `1.0+`, which implements Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md)

Attributes collected:

| Attribute    | Short Description                  | 
| ------------ | ---------------------------------- | 
| `http.route` | The matched route (path template). |
JamieDanielson commented 2 months ago

Okay, I left a comment on #2077 about this... sorry for the confusion heh but I'm okay with making the decision to omit the Notes column with the key. The attribute itself is the most important bit. @dyladan I guess let us know if you feel strongly about us keeping the Notes column, I think we've been kinda figuring it out as we go so I don't think there will be objection but wanted to check since you were the first to create this issue. Also maybe @pichlermarc