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

feat: mark run activity span as consumer span #1500

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

Conversation

remitamine
Copy link

What was changed

the arguments passed to instrument function in OpenTelemetryActivityInboundInterceptor's execute method has been expended which would be passed to the Open Telemetry startActiveSpan method.

Why?

this marks the run activity spans as consumer spans and adds a value to the messaging system field.
the information in itself is useful and it's needed in some cases for observability software such as Elastic Observability when apm-agent-nodejs is used with OpenTelemetry bridge where those fields are checked to properly map the span to a messaging transaction.

@remitamine remitamine requested a review from a team as a code owner August 17, 2024 21:41
@CLAassistant
Copy link

CLAassistant commented Aug 17, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mjameswh
Copy link
Contributor

@remitamine Can you please sign the CLA? I'll have to decline this PR otherwise.

@remitamine
Copy link
Author

@mjameswh one issue I'm having with this is that the @CLAassistant is requesting permissions that i don't understand why would they be necessary, especially about the access to private contributions(even if it's partial) and the ability to modify the contributions.
image

@mjameswh
Copy link
Contributor

one issue I'm having with this is that the @CLAassistant is requesting permissions that i don't understand why would they be necessary, especially about the access to private contributions(even if it's partial) and the ability to modify the contributions.

Oh, that's unexpected. I totally understand your concern.

For context, CLA Assistant is developed and operated by SAP. We are mere users of that service. The list of permissions the service is requesting are those that are normally required from a project's owner (i.e. the user or organization owning the project to be covered by the CLA). As a contributor, you should only be required for permission to read your email address.

Now, there seems to be some long outstanding issues in CLA Assistant that makes it sometime request for more permissions than it should. One known case of this is having cookie disabled. Could this be your case? Also, make sure to access the CLA signature page using this URL (which includes the repo name AND the PR number); I believe any other route will get you to the project administrator interface, which will trigger a request for extended GH permissions.

@remitamine
Copy link
Author

one issue I'm having with this is that the @CLAassistant is requesting permissions that i don't understand why would they be necessary, especially about the access to private contributions(even if it's partial) and the ability to modify the contributions.

Oh, that's unexpected. I totally understand your concern.

For context, CLA Assistant is developed and operated by SAP. We are mere users of that service. The list of permissions the service is requesting are those that are normally required from a project's owner (i.e. the user or organization owning the project to be covered by the CLA). As a contributor, you should only be required for permission to read your email address.

Now, there seems to be some long outstanding issues in CLA Assistant that makes it sometime request for more permissions than it should. One known case of this is having cookie disabled. Could this be your case? Also, make sure to access the CLA signature page using this URL (which includes the repo name AND the PR number); I believe any other route will get you to the project administrator interface, which will trigger a request for extended GH permissions.

i guess it might be related to the cookies, although they are not completely disabled i do have add-ons that control the cookies.
anyway, using the URL that you've posted worked this time without additional permissions.

@mjameswh
Copy link
Contributor

mjameswh commented Nov 15, 2024

Thanks for signing in the CLA!

Now, regarding the changes you propose in this PR, I think that SpanKind.CLIENT and SpanKind.Server would better reflect the relationship between a Workflow and an Activity. The thing here is that, from the Workflow's perspective, the Activity executes synchronously (i.e. the Workflow is generally waiting for the Activity to complete, see comments here).

It is true that, under the hood, the Temporal server will actually queue up a task for later execution, but that's an internal detail.

What do you think?

@remitamine
Copy link
Author

Thanks for signing in the CLA!

Now, regarding the changes you propose in this PR, I think that SpanKind.CLIENT and SpanKind.Server would better reflect the relationship between a Workflow and an Activity. The thing here is that, from the Workflow's perspective, the Activity executes synchronously (i.e. the Workflow is generally waiting for the Activity to complete, see comments here).

It is true that, under the hood, the Temporal server will actually queue up a task for later execution, but that's an internal detail.

What do you think?

based on my understanding of temporal and the semantic conventions for both HTTP and Messaging spans, i think that Messaging spans fit more into Temporal's having a server(intermediary) that receives messages from a producer and forwards the messages into consumers, in the this case the activity is processing the messages forwarded from the temporal server after receiving start activity messages from the producer, and the processing is not necessary immediate(it's queued), the queuing of the messages is actually crucial to make the distinction between HTTP/RPC spans(that fit into client and server types where the messages are supposed to be process immediately) and the messaging spans(that fit into the producer and consumer types where the messages can be delayed based on the queue).

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

Successfully merging this pull request may close these issues.

3 participants