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

fix: Make Spans initiated in Instrumentation for HTTP Frameworks of Kind Server #2307

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

XVincentX
Copy link

@XVincentX XVincentX commented Jun 27, 2024

Which problem is this PR solving?

The Span Kind in the all the HTTP framework instrumentations is not specified, making it internal by default.

A lot of providers, including Azure, is not reporting exceptions events in the spans if marked as internal.

Make sure the span is of kind server for exception tracking purposes
@XVincentX XVincentX requested a review from a team June 27, 2024 18:39
@github-actions github-actions bot requested a review from rauno56 June 27, 2024 18:39
@XVincentX XVincentX changed the title Make Restify Span kind server fix: Make Restify Span kind server Jun 27, 2024
@XVincentX XVincentX changed the title fix: Make Restify Span kind server fix: Make Restify Instrumentation Span kind server Jun 27, 2024
@XVincentX XVincentX changed the title fix: Make Restify Instrumentation Span kind server fix: Make HTTP Frameworks Instrumentation Span Kind server Jun 27, 2024
@XVincentX XVincentX changed the title fix: Make HTTP Frameworks Instrumentation Span Kind server fix: Make Spans initiated in Instrumentation for HTTP Frameworks of Kind Server Jun 27, 2024
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

It would be nice to get update the tests for all of these changes so we can enforce it doesn't regress in the future.

@github-actions github-actions bot added the pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. label Jun 28, 2024
@pichlermarc pichlermarc added the bug Something isn't working label Jun 28, 2024
@pichlermarc
Copy link
Member

pichlermarc commented Jun 28, 2024

I think that this behavior is intentional.

AFAIK in this context only http spans are supposed to be SpanKind.SERVER (these are generated by @opentelemetry/instrumentation-http). The extra spans generated by @opentelemetry/instrumentation-express, @opentelemetry/instrumentation-fastify, @opentelemetry/instrumentation-restify are all SpanKind.INTERNAL becaue they don't directly communicate with something else.

So any kind of a middleware span is an internal span (it's neither server nor client, it's in the middle -> internal).

From the spec:

  • SERVER Indicates that the span covers server-side handling of a synchronous RPC or other remote request. This span is often the child of a remote CLIENT span that was expected to wait for a response.
  • CLIENT Indicates that the span describes a request to some remote service. This span is usually the parent of a remote SERVER span and does not end until the response is received.
    [...]
  • INTERNAL - Default value. Indicates that the span represents an internal operation within an application, as opposed to an operations with remote parents or children.

this applies here, the middleware span does not have any remote parents or children, only the http span does.

@trentm
Copy link
Contributor

trentm commented Jun 28, 2024

I think that this behavior is intentional.

I agree.

@XVincentX Does pichlermarc's explanation above make sense?

Can you provide more details on:

A lot of providers, including Azure, is not reporting exceptions events in the spans if marked as internal.

@XVincentX
Copy link
Author

@trentm I am no expert enough in OpenTelemetry to say whether that is correct or not - I thought it was a bug, but I believe your assessment.

Can you provide more details

https://github.com/Azure/azure-sdk-for-js/blob/f4d9a79bc07cc538b0867ede7760da15d0cb523a/sdk/monitor/monitor-opentelemetry-exporter/src/utils/spanUtils.ts#L336

@pichlermarc
Copy link
Member

I believe this may be an issue that's better suited for the Azure SDK repo 🤔
It's seems related to the mapping of OTel to Azure Monitor, which is out of the realm of our expertise.

cc @hectorhdzg IIRC you're working on the JS Azure SDK, right? 🤔

@hectorhdzg
Copy link
Member

@XVincentX can you describe your scenario and what exactly are you trying to achieve in Azure Monitor side? are you expecting to see all instrumentation errors in your exception table in Application Insights? I don't believe any instrumentation different to HTTP currently call recordException in the Span, so even making the span internal you will not get exception telemetry as you are expecting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:instrumentation-express pkg:instrumentation-fastify pkg:instrumentation-restify pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants