refactor(attributes): move enums to @opentelemetry/semantic-conventions

[markwolff]

Which problem is this PR solving?

• Closes [Refactor][Plugins] Move common attributes to Core/Base #522

Short description of the changes

• Moves common span attributes names defined by specs to @opentelemetry/semantic-conventions package.

https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/trace/semantic_conventions

[markwolff]

Actually it looks like #718 already exists. I can close this in favor of that one if we are able to resolve the conflicts there

[dyladan]

Looks like #718 is stale since january

[mayurkale22]

Nice 👍

[markwolff]

re: #718 (review), was there ever alignment on this living in core vs api? I think there's a good case for it living in api, since these will primarily be useful for library / application owners using the APIs.

I agree with const enum over enum, as long as we are okay with this remaining a flat structure, since that is the only way to use const enums. We could create separate "structured" vars to solve for this though

const enum AttributeNames { ... }

const db = {
  required: {
    statement: AttributeNames.DB_STATEMENT,
  }
}

edit: looks like using const enum will break a lot of use cases. For example, to get our own tests to pass w/ ts-mocha I needed to prefix with TS_NODE_TYPE_CHECK=true

[codecov]

Codecov Report

Merging #1160 into master will decrease coverage by 0.20%.
The diff coverage is 81.48%.

@@            Coverage Diff             @@
##           master    #1160      +/-   ##
==========================================
- Coverage   92.37%   92.17%   -0.21%     
==========================================
  Files         119      120       +1     
  Lines        3447     3410      -37     
  Branches      700      697       -3     
==========================================
- Hits         3184     3143      -41     
- Misses        263      267       +4     

Impacted Files	Coverage Δ
...lemetry-semantic-conventions/src/trace/database.ts	0.00% <0.00%> (ø)
...elemetry-semantic-conventions/src/trace/general.ts	0.00% <0.00%> (ø)
...entelemetry-semantic-conventions/src/trace/http.ts	0.00% <0.00%> (ø)
...pentelemetry-semantic-conventions/src/trace/rpc.ts	0.00% <0.00%> (ø)
packages/opentelemetry-plugin-http/src/utils.ts	97.42% <93.33%> (ø)
packages/opentelemetry-plugin-grpc/src/grpc.ts	96.79% <100.00%> (ø)
packages/opentelemetry-plugin-http/src/http.ts	97.90% <100.00%> (ø)
...s/opentelemetry-plugin-xml-http-request/src/xhr.ts	79.31% <100.00%> (ø)
... and 2 more

[dyladan]

Unfortunately const enum has a lot of issues. It also doesn't play well with some bundlers for some reason. I think it might make sense to have these constants live in a separate package. Also, the spec has been working on a code generator for semantic convention constants that we will eventually want to migrate over to use, so I would caution against pushing this into the API only to have it removed later.

[markwolff]

I think it might make sense to have these constants live in a separate package. Also, the spec has been working on a code generator for semantic convention constants that we will eventually want to migrate over to use, so I would caution against pushing this into the API only to have it removed later.

I agree code generation seems like a much better option than baking it into api and I imagine the same thinking applies to core as well. If so, this issue&PR should just be closed / changed to a tracking issue for that eventual addition. WDTY?

[dyladan]

For now, I would say if we're going to centralize the constants, we should do so in a new package @opentelemetry/semantic-conventions or similar which can be deprecated or updated when the code gen is available

[mwear]

LGTM.

I will point out that the component semantic convention was removed: open-telemetry/opentelemetry-specification#447. We can handle its removal separately.