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

OpenTelemetry: NPE in HttpInstrumenterVertxTracer.RouteGetter when request got cancelled #26779

Merged
merged 1 commit into from
Jul 25, 2022

Conversation

tmihalac
Copy link
Contributor

Fixes #26764

Signed-off-by: Theodor Mihalache [email protected]

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 18, 2022

/cc @radcortez

Copy link
Member

@radcortez radcortez left a comment

Choose a reason for hiding this comment

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

There are other places we call response.get*, so maybe we need to address those.

@gsmet
Copy link
Member

gsmet commented Jul 18, 2022

@radcortez but will you call the methods of ServerAttributesExtractor if you don't have a response? Because AFAICS, the others are in the extractor.

@radcortez
Copy link
Member

Even when we don't have a response, we need to call instrumenter.end(...) to finish the span. Inside, OTel will call the extractor to fill in the tags in the span, also calling the ones that require a Response. We don't have control over this.

@gsmet
Copy link
Member

gsmet commented Jul 19, 2022

@tmihalac can you adjust and also be null safe for the methods in ServerAttributesExtractor that are manipulating response? From what I can see, you can return null if the response is null.

@tmihalac
Copy link
Contributor Author

@tmihalac can you adjust and also be null safe for the methods in ServerAttributesExtractor that are manipulating response? From what I can see, you can return null if the response is null.

So all I need to do is to be null safe with the response in ServerAttributesExtractor ?

@quarkus-bot

This comment has been minimized.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Apart from my small comment, I think it makes sense.

But I will let @radcortez have the final say.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Jul 20, 2022

For the formatting issues, I recommend you have a look at: https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#ide-config-and-code-style .
And also run the formatter before pushing if you have doubts. But frankly, you should always build before pushing to check things at least compile.

Also, time for you to learn how to squash your commits yourself :).

What I personally do is:

git rebase -i <hash of the commit before my first change>

This will open an editor.
I keep p for the first commit and put f for the commit I want to squash. Then close the editor and Git should squash the commits. You can then force push to your branch.

Note: be careful when doing that as you rewrite the history.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

…quest got cancelled

Fixed NPE in HttpInstrumenterVertxTracer$RouteGetter when request got cancelled

Signed-off-by: Theodor Mihalache <[email protected]>
@radcortez
Copy link
Member

Thank you!

@gsmet gsmet merged commit b2574a8 into quarkusio:main Jul 25, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Jul 25, 2022
@gsmet gsmet modified the milestones: 2.12 - main, 2.11.1.Final Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenTelemetry: NPE in HttpInstrumenterVertxTracer.RouteGetter when request got cancelled
3 participants