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

Pass OpenTelemetry to SystemAccessControl #18899

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Sep 2, 2023

This is necessary for SystemAccessControl implementation to participate in tracing. Useful when the implementation does e.g. network communication when performing access control.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Sep 2, 2023
@cla-bot cla-bot bot added the cla-signed label Sep 2, 2023

interface SystemAccessControlContext
{
OpenTelemetry getOpenTelemetry();
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need an instance of OpenTelemetry? Generally, all you need is a properly configured Tracer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't see how access control needs would be different than those of a connector. I followed ConnectorContext where @electrum added both

default OpenTelemetry getOpenTelemetry()
{
throw new UnsupportedOperationException();
}
default Tracer getTracer()
{
throw new UnsupportedOperationException();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find any authoritative docs or code comments, thus may be drawing wrong conclusions, but both are expected here
https://github.com/airlift/airlift/blob/c55517863fe246f8921b0a03034b0e2f0d7a31c9/http-client/src/main/java/io/airlift/http/client/HttpClientModule.java#L121-L131
and both are later used, if provided.

This is necessary for SystemAccessControl implementation to participate
in tracing. Useful when the implementation does e.g. network
communication when performing access control.
{
return new SystemAccessControlContext()
{
private final Tracer tracer = openTelemetry.getTracer("trino.system-access-control." + systemAccessControlName);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: verify that systemAccessControlName is not empty or null

Copy link
Member Author

Choose a reason for hiding this comment

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

this is verified by the caller within the class and the method is private
also, empty string is currently valid factory name :)

@findepi findepi merged commit c76b103 into trinodb:master Sep 5, 2023
@findepi findepi deleted the findepi/sac-trace branch September 5, 2023 09:06
@github-actions github-actions bot added this to the 426 milestone Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

3 participants