Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump 2.16 branch to OTel 1.23 and SR reactive messaging 3.24 #31840

Closed
wants to merge 2 commits into from

Conversation

brunobat
Copy link
Contributor

No description provided.

@brunobat brunobat self-assigned this Mar 14, 2023
@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Mar 14, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 14, 2023

/cc @radcortez (opentelemetry)

@gsmet
Copy link
Member

gsmet commented Mar 14, 2023

What's the rationale for backporting this bump?

@brunobat
Copy link
Contributor Author

It was asked by a user, but @ozangunalp has more details.

@ozangunalp
Copy link
Contributor

I can't find the discussion right now. The reactive messaging 3.24.0 also includes the fix for #31003.

@gsmet
Copy link
Member

gsmet commented Mar 14, 2023

OK, makes sense then if not too risky. Thanks for the clarification.

@quarkus-bot

This comment has been minimized.

@ozangunalp
Copy link
Contributor

Test failures seem to be related. I am looking at it.

Otel Kafka message offset attribute changed to attr_messaging.kafka.message.offset
@ozangunalp
Copy link
Contributor

The Kafka record offset Otel attribute changed in the previous Reactive Messaging release.
I also backported that commit.

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 15, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@gsmet
Copy link
Member

gsmet commented Mar 20, 2023

I had a look at all the tests you had to adjust and this PR actually worries me a lot. Is it really safe to backport that to 2.16?

@radcortez
Copy link
Member

These are related to two significant changes from OTel:

The problematic piece is the method renames. They just renamed the methods, and there was no deprecation cycle. These could break user applications if they are using these APIs. My gut feeling is that the chance is small because these APIs are mostly for libraries integration, but we should be aware of this.

@gsmet
Copy link
Member

gsmet commented Mar 20, 2023

@radcortez yeah so I knew about these changes, my problem is if they are safe enough to be backported to 2.16 given we are already late in the 2.16 cycle so people are expecting stability.

@radcortez
Copy link
Member

Yes, I agree that these changes should not be expected in a patch release.

Even with the span name change, it would look weird to start seeing different span names in your tools. We could override their span name creator to keep the old behavior for this version. On the other hand, it would also look weird because this OTel version changes the name, and we are changing it again :)

Can we clarify what the user needs to ask for the backport?

@brunobat
Copy link
Contributor Author

This started on a SM Reactive messaging backport request. Because OTel 1.23 is a dependency, it requires Otel backport due the mentioned changes, this was created.
If @ozangunalp is happy not to backport we can scrap this.

@ozangunalp
Copy link
Contributor

I agree with the stability argument. No problem for me to scrap this PR.
In any case, I can't find the original user request.

@brunobat
Copy link
Contributor Author

Dropping it.

@brunobat brunobat closed this Mar 20, 2023
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Mar 20, 2023
@brunobat brunobat deleted the bump-2-16-to-otel-1.23 branch August 21, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/reactive-messaging area/tracing triage/invalid This doesn't seem right
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants