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

gateway: add operation_name field for task polling #1590

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

iulianbarbu
Copy link
Contributor

@iulianbarbu iulianbarbu commented Jan 26, 2024

Description of change

Each new project task gets created in the context of a parent span. With open telemetry, this boils down to spans linked through context propagation, which can be easily visualized with Datadog in APM Traces. What is difficult though is to filter spans based on the relationship between them (which should get easier after the GA of [1], thanks @Kazy for pointing me to the article), so that only a certain selection of spans can be returned and counted for certain metrics.

In my specific case, I want to filter for all spans created by the poll function of a ProjectTask that has an attribute called ctx.operation_name. This attribute is attached to the span only in case the result of the poll function is TaskResult::Done(_). This is relevant because ProjectTasks are created and driven in the background as a result of gateway APIs handlers, the ambulance, and find_or_start_project. For now, the only difference I am interested in is determining the number of spans that have the ctx.operation_name attribute equal to create_project. In this way, Datadog will also be able to count all ProjectTasks that end with a TaskResult::Done(_) and that is started by an operation called create_project.

How has this been tested? (if applicable)

Tested with Jaeger, and the field shows up.

@iulianbarbu iulianbarbu force-pushed the add_operation_name_record branch 2 times, most recently from 394411a to 09f5657 Compare January 26, 2024 15:30
Copy link
Contributor

@Kazy Kazy left a comment

Choose a reason for hiding this comment

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

This works for this particular problem, but I had in mind a broader solution to that problem, such that every downstream span would inherit that attribute. While I was thinking about doing it ourselves with Layer/Context, it seems like this already exists in the OPTL world under the name of Baggage.

This could be time consuming to implement though, so maybe we want to trade the speed of implementation and revisit the more general implementation latter.

@iulianbarbu
Copy link
Contributor Author

This works for this particular problem, but I had in mind a broader solution to that problem, such that every downstream span would inherit that attribute. While I was thinking about doing it ourselves with Layer/Context, it seems like this already exists in the OPTL world under the name of Baggage.

This could be time consuming to implement though, so maybe we want to trade the speed of implementation and revisit the more general implementation latter.

Didn't know about Baggage and yes, it looks like what we need, but I am not sure how that would work out with DataDog.
I also think figuring out how to use a Layer or Context to make context propagation propagate an attribute that will be shared by an entire distributed trace is also an option, but I am not sure about a good way to implement it, so any ideas are welcome. In the meantime, I think this solution should unblock us for our use case (after I confirm it works locally).

@iulianbarbu iulianbarbu marked this pull request as ready for review January 26, 2024 17:59
Copy link
Contributor

@Kazy Kazy left a comment

Choose a reason for hiding this comment

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

LGTM !

@iulianbarbu iulianbarbu merged commit 625a015 into shuttle-hq:main Jan 29, 2024
36 checks passed
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.

2 participants