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(telemetry): add option to omit context propagation #2946

Merged
merged 10 commits into from
Dec 10, 2024

Conversation

fgozdz
Copy link
Contributor

@fgozdz fgozdz commented Nov 29, 2024

ref #2942

@fgozdz fgozdz requested a review from manast November 29, 2024 21:10
@roggervalf
Copy link
Collaborator

Wondering if an extra option can be passed at job level as for the case that was reportes, only repeatables jobs in some cases need this logic but we can keep the base logic for all the other job types

@manast
Copy link
Contributor

manast commented Dec 2, 2024

Wondering if an extra option can be passed at job level as for the case that was reportes, only repeatables jobs in some cases need this logic but we can keep the base logic for all the other job types

In this particular case it was not really repeatable jobs, they were simulating repetition using custom code. However the point is valid, maybe it is better to have it as a per job config, worth to think more about it.

@fgozdz fgozdz marked this pull request as draft December 4, 2024 13:42
@fgozdz
Copy link
Contributor Author

fgozdz commented Dec 4, 2024

I have changed it to pass the option entirely in bullmq, inside the job. For now only in methods regarding enhancement issue.

@fgozdz fgozdz requested a review from roggervalf December 4, 2024 13:45
@manast manast marked this pull request as ready for review December 7, 2024 10:01
Copy link
Contributor Author

@fgozdz fgozdz left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -404,7 +404,10 @@ export class Queue<
...this.jobsOpts,
...job.opts,
jobId: job.opts?.jobId,
tm: span && srcPropagationMedatada,
tm:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused, why is the option used here "tm" instead of "telemetryMetadata" as on the other functions above?

@manast
Copy link
Contributor

manast commented Dec 10, 2024

I will squash this PR as there were a few commits on this one...

@manast manast merged commit 6514c33 into master Dec 10, 2024
12 checks passed
@manast manast deleted the feat/telemetry-option-omit-context branch December 10, 2024 14:32
github-actions bot pushed a commit that referenced this pull request Dec 10, 2024
# [5.34.0](v5.33.1...v5.34.0) (2024-12-10)

### Features

* **telemetry:** add option to omit context propagation on jobs ([#2946](#2946)) ([6514c33](6514c33))
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