Skip to content

Revert the switch to default to dup semantic conventions for http packages#7317

Closed
dmathieu wants to merge 1 commit into
open-telemetry:mainfrom
dmathieu:revert-http-dup
Closed

Revert the switch to default to dup semantic conventions for http packages#7317
dmathieu wants to merge 1 commit into
open-telemetry:mainfrom
dmathieu:revert-http-dup

Conversation

@dmathieu
Copy link
Copy Markdown
Member

This reverts #6899, as we've had several bug fixes we needed to bring into the main branch and waiting for an additional release will put less pressure on folks.

See SIG discussions: open-telemetry/opentelemetry-go#6648
Closes #7269

@github-actions github-actions Bot requested review from akats7, dashpole and flc1125 May 12, 2025 12:41
@dmathieu dmathieu changed the title Severt the switch to default to dup semantic conventions for http packages Revert the switch to default to dup semantic conventions for http packages May 12, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2025

Codecov Report

Attention: Patch coverage is 97.56098% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.1%. Comparing base (cc803c5) to head (72d40e6).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...lei/go-restful/otelrestful/internal/semconv/env.go 96.9% 1 Missing ⚠️
....com/gin-gonic/gin/otelgin/internal/semconv/env.go 96.9% 1 Missing ⚠️
...ub.com/gorilla/mux/otelmux/internal/semconv/env.go 96.9% 1 Missing ⚠️
...tp/httptrace/otelhttptrace/internal/semconv/env.go 96.9% 1 Missing ⚠️
...entation/net/http/otelhttp/internal/semconv/env.go 96.9% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #7317     +/-   ##
=======================================
- Coverage   81.2%   81.1%   -0.2%     
=======================================
  Files        207     207             
  Lines      18263   18123    -140     
=======================================
- Hits       14832   14699    -133     
+ Misses      3009    2981     -28     
- Partials     422     443     +21     
Files with missing lines Coverage Δ
...o-restful/otelrestful/internal/semconv/httpconv.go 87.4% <100.0%> (-1.6%) ⬇️
...gin-gonic/gin/otelgin/internal/semconv/httpconv.go 87.4% <100.0%> (-3.0%) ⬇️
...m/gorilla/mux/otelmux/internal/semconv/httpconv.go 87.4% <100.0%> (-1.6%) ⬇️
...tptrace/otelhttptrace/internal/semconv/httpconv.go 87.4% <100.0%> (-1.6%) ⬇️
...ion/net/http/otelhttp/internal/semconv/httpconv.go 87.4% <100.0%> (-1.6%) ⬇️
...lei/go-restful/otelrestful/internal/semconv/env.go 94.7% <96.9%> (ø)
....com/gin-gonic/gin/otelgin/internal/semconv/env.go 94.7% <96.9%> (ø)
...ub.com/gorilla/mux/otelmux/internal/semconv/env.go 92.7% <96.9%> (ø)
...tp/httptrace/otelhttptrace/internal/semconv/env.go 90.1% <96.9%> (ø)
...entation/net/http/otelhttp/internal/semconv/env.go 90.1% <96.9%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dmathieu dmathieu marked this pull request as ready for review May 12, 2025 13:50
@dmathieu dmathieu requested a review from a team as a code owner May 12, 2025 13:50
@dmathieu dmathieu added this to the v1.36.0 milestone May 13, 2025
Copy link
Copy Markdown
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this change will a user get both v1.20.0 and v1.26.0 attributes or will they only get v1.20.0 attributes by default?

@dmathieu
Copy link
Copy Markdown
Member Author

They will get v1.20.0 only by default, same as on the latest release. http/dup gives them both.

@pellared
Copy link
Copy Markdown
Member

pellared commented May 14, 2025

@dmathieu, could you please remind why do you think that we should revert? Given the frequency of the releases I think that maybe we should just go forward?

We already have a warning in the latest release https://github.com/open-telemetry/opentelemetry-go-contrib/releases/tag/v1.35.0:

This is the last version to use Semantic Conventions v1.20.0 for HTTP libraries by default. The next version (0.61.0) will default to v1.26.0, and the following one (0.62.0) will drop support for Semantic Conventions v1.20.0

Was there any user request that we should postpone?

Given the message above our users may be frustrated that we do not keep our promises if we do the revert. Do you plan to make a v0.60.1 patch release before releasing v0.62.0?

PS. Sorry my comment, but I have second thoughts 😬

@dmathieu
Copy link
Copy Markdown
Member Author

There have been several bugfixes and issues discovered that will only be introduced with this new release.
Eg: #6942
But also #6951 (which is more problematic, as folks can't use http/dup at all right now).

Hence my proposal to postpone for one release (minor, not patch). I suggested that during a SIG meeting and folks are onboard with it, but I'm fine revisiting that too.

@pellared
Copy link
Copy Markdown
Member

pellared commented May 14, 2025

There have been several bugfixes and issues discovered that will only be introduced with this new release.

Sure, but I do not think it means that we cannot introduce a change that was planned and announced. Users who care about these bugfixes would simply need to switch to use v1.26.0 sem conv or set OTEL_SEMCONV_STABILITY_OPT_IN=http/dup

But also #6951 (which is more problematic, as folks can't use http/dup at all right now).

Is it only some issue for otelrestful? It looks like it works for otelhttp (#7321). Do we have some bug for it? Do we have any way to move forward with the issue and would postponing help in any way?

I suggested that during a SIG meeting

I was not present at open-telemetry/opentelemetry-go#6648 (comment). I am sorry that I have not provided any feedback. I think I was too much focused on making the notes when watching the recording.

I'm fine revisiting that too.

I think we should decide no later than during next SIG meeting. At the same time we need discuss it beforehand. I do not say that we should delay any conversations as they are necessary to get to an agreement during the SIG meeting.

@dmathieu
Copy link
Copy Markdown
Member Author

Yes, it's only otelrestful which didn't use the semconv package at all.

@pellared
Copy link
Copy Markdown
Member

pellared commented May 14, 2025

Yes, it's only otelrestful which didn't use the semconv package at all.

I think it is a very minor issue. I wouldn't be supprised if you were the only one who noticed the issue. I do not expect that even having support for http/dup would help as (if I am not mistaken) nobody even filled an issue that otelrestful does not support OTEL_SEMCONV_STABILITY_OPT_IN.

Honestly speaking, I also consider otelrestful module a second-class citizen compared to otelhttp. I would not want to slow down our progress on otelhttp becasue of otelrestful.

@dmathieu
Copy link
Copy Markdown
Member Author

We have decided in the SIG meeting not to do this revert.

@dmathieu dmathieu closed this May 15, 2025
@dmathieu dmathieu deleted the revert-http-dup branch May 15, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Revert switching the HTTP semconv migration's default to new

3 participants