semconv: mark the package as deprecated

[codeboten]

This package will be removed in the future, use go.opentelemetry.io/otel/semconv instead

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.58%. Comparing base (ee2c784) to head (ef2188d).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13012   +/-   ##
=======================================
  Coverage   91.58%   91.58%           
=======================================
  Files         505      505           
  Lines       28319    28319           
=======================================
  Hits        25937    25937           
  Misses       1873     1873           
  Partials      509      509           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bogdandrutu]

Please explain how can we use that in the collector pdata world where the otel attributes (and KeyValues, etc.) are incompatible

[codeboten]

@bogdandrutu sure, i created the following PRs to update the core/contrib repos to use the otelgo semconv package:

• update semconv dependency to otelgo's semconv package #12991
• [chore] update tests semconv dep opentelemetry-collector-contrib#39979
• [chore] update dependency to use otelgo semconv opentelemetry-collector-contrib#39986

It seemed to work ok. See the comment in the original PR:

This updates the calls to the collector's internal semconv package with otelgo's instead. The main difference is how the keys can be used, they will need to be cast as strings where they key name is needed, otherwise it's not a huge change. This would allow us to stop producing our own semconv package once contrib is moved as well.

I ran some benchmarks of instantiating the service's Resource using the old semconv and the new ones to validate that using the new semconv wouldn't have an impact on allocations:

benchstat 20_runs_bench.txt 20_oldsemconv_runs_bench.txt 
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/collector/service/internal/resource
cpu: Apple M3 Max
               │ 20_runs_bench.txt │    20_oldsemconv_runs_bench.txt    │
               │      sec/op       │   sec/op     vs base               │
NewResource-14         332.2n ± 2%   323.4n ± 1%  -2.68% (p=0.000 n=20)

               │ 20_runs_bench.txt │  20_oldsemconv_runs_bench.txt  │
               │       B/op        │    B/op     vs base            │
NewResource-14          720.0 ± 0%   720.0 ± 0%  ~ (p=1.000 n=20) ¹
¹ all samples are equal

               │ 20_runs_bench.txt │  20_oldsemconv_runs_bench.txt  │
               │     allocs/op     │ allocs/op   vs base            │
NewResource-14          8.000 ± 0%   8.000 ± 0%  ~ (p=1.000 n=20) ¹
¹ all samples are equal

Is there a specific aspect of pdata using the otel go's semconv that you're thinking about?

[bogdandrutu]

@codeboten

Are we concerned about string(semconv.ServiceNameKey) being a bit too verbose/cryptic? Should we, if this is a pattern we want, at least extend the pcommon.Map to have some overloads that accepts as key the otel attrs? Or at least something like "PutKeyValue" for example?

I understand there is no performance problem, it is just the semantics and how code looks like that concerns me.

[mx-psi]

LGTM. I think the ergonomics suffer, but I think it is fine and that we can add the helpers if necessary.

If we don't do this we need to have a plan forward on maintaining semconv, we have been inconsistent in keeping it up to date.

It's also probably good to do this in terms of the binary size (historically I have heard complaints related to the amount of strings that our binaries have because of semconv)

[songy23]

Is this a release blocker ?

[TylerHelmuth]

I agree with the approach of using otel-go instead of updating this package to be generated via weaver. If the ergonomics of string() gets too annoying we can add new functions to pdata to improve the experience.

[codeboten]

Is this a release blocker ?

It doesn't have to block the release i don't think. We don't need to mark the semconv package as deprecated in this release

Looks like other are ok with the downgraded usability experience, I am fine to not block this.

[dmitryax]

LGTM

[mx-psi]

#13065 removes the associated workflow