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

adding header per request is not working #5959

Closed
shadow0wolf opened this issue Nov 3, 2023 · 4 comments · Fixed by #6004
Closed

adding header per request is not working #5959

shadow0wolf opened this issue Nov 3, 2023 · 4 comments · Fixed by #6004
Labels
Bug Something isn't working

Comments

@shadow0wolf
Copy link

shadow0wolf commented Nov 3, 2023

Describe the bug
The internal feature to add http headers on each telemetry signal request which is introduced through change described as : #4630 is not working .

'OkHttpHttpSender' class constructor invokes the supplier assigned in io.opentelemetry.exporter.internal.auth.Authenticator , but for this to work per request , shouldnt this be invoked in OkHttpHttpSender's send method.

Steps to reproduce
//create a Http Span Exporter as
builder = OtlpHttpSpanExporterBuilder.build() ;

//create Authenticator instance as
io.opentelemetry.exporter.internal.auth.Authenticator supplier = ()->{
System.out.println("@#@# invoking supplier function to generate headers");
String key = "my-custom-header";
String val = "123123123";
return Collections.singletonMap(key,val);
};
//assign this authenticator instance to builder
Authenticator.setAuthenticatorOnDelegate(builder,supplier)

If you instrument any application with above settings , the OTEL http request is supposed to contain header : my-custom-header : 123123123 , but this header is not visible in the telemetry signals.

What did you expect to see?
The supplier defined above must invoke for each request and the header supplied must be visible in telemetry request .

What did you see instead?
Header is not visible in span telemetry signal request

What version and what artifacts are you using?
Artifacts: ( opentelemetry-sdk )
Version: (1.31.0 )

Related issues :
#4292

@jack-berg
Copy link
Member

Authenticator.getHeaders() is only invoked if the client receives a 401 response. See this unit test to see the sequence of events: https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractHttpTelemetryExporterTest.java#L344-L371

  • Client sends a request with no headers
  • Client receives 401 response
  • Client retries request the the headers obtained from Authenticator#getHeaders

This concept was modeled after OkHttp Authenticator.

@shadow0wolf
Copy link
Author

@jack-berg , is the header obtained after invocation of supplier function getHeaders() sent in subsequent telemetry requests or only the telemetry request .

@jack-berg
Copy link
Member

@jack-berg , is the header obtained after invocation of supplier function getHeaders() sent in subsequent telemetry requests or only the telemetry request .

I just wrote a test on this and confirmed that the header obtained from Authenticator#getHeaders is only used on the specific request that received the 401 response.

Its a weird concept TBH. If you're connecting to a server that requires authentication, you're going to get a 401 every time. So its wasteful to make an unauthenticated request just so you can be prompted to include an Authorization header.

What I think we really want is the ability to set a supplier of headers, instead of fixed static ones, and also give the ability for a particular export attempt to retry after refreshing the authorization header in the event it receives a 401 back. I think we
want to enable something like this:

AtomicReference<String> token = new AtomicReference<>();
token.set(refreshToken());

OtlpHttpSpanExporter exporter = OtlpHttpSpanExporter.builder()
  .setHeaderSupplier(() -> Map.of("Authorization", token.get()))
  .setAuthenticator(() -> {
    String newToken = refreshToken();
    token.set(newToken);
    return Map.of("Authorization", newToken);
  })
  .build();

@shadow0wolf
Copy link
Author

@jack-berg , exactly , we need the supplier method that adds headers to be in the send method of senders class (eg OKHttpSender) , plus the ability to set the authenticator when creating the Exporter , this is how i had to write my custom code to make my use-case specific the authentication scheme work .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants