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

EndUserSpanProcessor integration #34595

Merged
merged 1 commit into from
Jul 27, 2023
Merged

EndUserSpanProcessor integration #34595

merged 1 commit into from
Jul 27, 2023

Conversation

amoscatelli
Copy link
Contributor

@amoscatelli amoscatelli commented Jul 7, 2023

Closes #34526

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 7, 2023

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

@amoscatelli amoscatelli force-pushed the main branch 2 times, most recently from 77d302a to 47e3f32 Compare July 7, 2023 07:47
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @amoscatelli.
I think we should not add a SpanProcessor and include a ManagedExecutor just for this purpose.
I was expecting to instrument quarkus-jwt and get that information from there.
I wonder if @sberyozkin can help with that?
We also need tests.

@quarkus-bot

This comment has been minimized.

@amoscatelli amoscatelli requested a review from brunobat July 8, 2023 08:45
@amoscatelli
Copy link
Contributor Author

Thanks for the PR @amoscatelli. I think we should not add a SpanProcessor and include a ManagedExecutor just for this purpose. I was expecting to instrument quarkus-jwt and get that information from there. I wonder if @sberyozkin can help with that? We also need tests.

I think there is a misunderstanding.
Even if you inject JsonWebToken principal, you still would require a ManagedExecutor to access it.
That was my starting point, but since the beginning vertx complained about being blocked, so I had to introduce a ManagedExecutor. Probably that's because the spanprocessor, accessing the JsonWebToken, transitively access the request and so it requires RequestScope and this operation becomes blocking.

Then I switched to SecurityIdentity, hoping, among the other things, to workaroud the ManagedExecutor thing, but it is still required.
Anyway I think that using SecurityIdentity is still an upgrade, because it is agnostic of the kind of authentication/security extension you are going to use.
There is no reason to couple this with jwt ...

Of course as I said from the beginning I would like to avoid both the ManagedExecutor and the requestscope.
What do you suggest ?

@amoscatelli amoscatelli force-pushed the main branch 2 times, most recently from f1461af to 859a574 Compare July 8, 2023 09:43
@sberyozkin
Copy link
Member

LGTM, indeed, should not be tied up with the specific type of credentials or authentication mechanism.
It only should not be enabled by default IMHO which what this PR enforces

@sberyozkin sberyozkin self-requested a review July 26, 2023 15:24
@sberyozkin
Copy link
Member

@amoscatelli Can you please fix the conflict ?

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

Thanks @sberyozkin.
@amoscatelli can you please add tests?

@amoscatelli
Copy link
Contributor Author

LGTM, indeed, should not be tied up with the specific type of credentials or authentication mechanism. It only should not be enabled by default IMHO which what this PR enforces

Thank you.
Anyway this is not enabled by default.
Default value is false for the new configuration.

@amoscatelli amoscatelli force-pushed the main branch 2 times, most recently from 6fe7e1a to 5b504c7 Compare July 26, 2023 17:04
@quarkus-bot

This comment has been minimized.

@amoscatelli amoscatelli force-pushed the main branch 3 times, most recently from 8c53459 to 1739282 Compare July 27, 2023 08:56
@amoscatelli amoscatelli requested a review from brunobat July 27, 2023 08:58
@amoscatelli
Copy link
Contributor Author

Thanks @sberyozkin. @amoscatelli can you please add tests?

Done !

@amoscatelli
Copy link
Contributor Author

@brunobat please tell me if there is something more I can do because this is requiring a rebase after rebase :-)

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

This looks great!
Thanks @amoscatelli

@quarkus-bot

This comment has been minimized.

@brunobat
Copy link
Contributor

@amoscatelli please build the extension and the IT test before pushing. The local build will auto format the code.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 27, 2023

✖ This workflow run has failed but no jobs reported an error. Something weird happened, please check the workflow run page carefully: it might be an issue with the workflow configuration itself.

@amoscatelli
Copy link
Contributor Author

amoscatelli commented Jul 27, 2023

@brunobat I think this is done, can this be merged please ?

@brunobat brunobat merged commit d7ae500 into quarkusio:main Jul 27, 2023
@brunobat
Copy link
Contributor

brunobat commented Jul 27, 2023

Thanks @amoscatelli. Do you have Twitter? I'd like to post about this PR.

@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 27, 2023
@amoscatelli
Copy link
Contributor Author

amoscatelli commented Jul 27, 2023

No sorry but I have Linkedin !
You can post on Twitter and tag me there, is this possible ?

@brunobat
https://www.linkedin.com/in/alessandromoscatelli/

@brunobat
Copy link
Contributor

sure

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.

OTEL - Automatic ENDUSER_ID and ENDUSER_ROLE filling
3 participants