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

Telemetry handler experience #1209

Open
1 of 2 tasks
isvargasmsft opened this issue Apr 18, 2023 · 9 comments
Open
1 of 2 tasks

Telemetry handler experience #1209

isvargasmsft opened this issue Apr 18, 2023 · 9 comments

Comments

@isvargasmsft
Copy link
Member

isvargasmsft commented Apr 18, 2023

2 things to address:

  • The telemetry handler would only set the client request Id if not already present, allowing people to set it manually if they need some kind of end-to-end tracing.
  • The telemetry handler should start a span and attach the client request id as a tag.
@ghost ghost added the ToTriage label Apr 18, 2023
@isvargasmsft
Copy link
Member Author

CC: @Ndiritu @SilasKenneth @shemogumbe

@Ndiritu
Copy link
Contributor

Ndiritu commented Apr 19, 2023

For context, our telemetry handler currently adds 2 headers to requests: SdkVersion and client-request-id. Client Request ID can be manually set or defaults to a random UUID.

The current dev experience of customising client request ID when using service libraries:

$telemetryOption = new GraphTelemetryOption();
$telemetryOption->setClientRequestId('custom id');

$requestConfig = new UsersRequestBuilderGetRequestConfiguration();
$requestConfig->options = [$telemetryOption];

$response = $graphServiceClient->users()->get($requestConfig)->wait();

Dev exp. when using Graph core only:

$telemetryOption = new GraphTelemetryOption();
$telemetryOption->setClientRequestId('custom id');

$guzzleClient = GraphClientFactory::setTelemetryOption($telemetryOption)::create();
$response = $guzzleClient->get('/users');

Some improvements/gaps in the dev experience:

  • can be shortened so that client request Id is set in the GraphTelemetryOption constructor
  • current design could allow devs to overwrite SDK versions. We can find a way to "hide" those.

@Ndiritu
Copy link
Contributor

Ndiritu commented Apr 19, 2023

@isvargasmsft the 2nd requirement isn't clear to me. Kindly clarify.

@isvargasmsft
Copy link
Member Author

@baywet I understand you have more context on these 2 requests, is that right? These came from Seb.

@baywet
Copy link
Member

baywet commented Apr 19, 2023

@Ndiritu Open Telemetry is not yet implemented in PHP as far as I know, correct?

@Ndiritu
Copy link
Contributor

Ndiritu commented Apr 19, 2023

Ah, didn't know we're referring to that.
Not yet.

@baywet
Copy link
Member

baywet commented Apr 19, 2023

right, so the second requirement can only be implemented when the tracing work is done. I suggest that you only implement the first now, and leave a note on the issue capturing tracing for the second.

@isvargasmsft
Copy link
Member Author

Thank you very much @baywet

@baywet
Copy link
Member

baywet commented Apr 19, 2023

here is the related issue I'm referring to microsoft/kiota#1871

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants