-
Notifications
You must be signed in to change notification settings - Fork 178
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(active-job): Honour dynamic changes in configuration #1079
fix(active-job): Honour dynamic changes in configuration #1079
Conversation
|
||
private | ||
|
||
def span_name(job) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be best to move this to the Default
class and also change all the other spans to have a consistent naming, but I'm not sure if we want to lump this change in the PR, this was merely intended to fix a regression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate PR to do just that: #1080
cb914fd
to
0aa2203
Compare
cb76703
to
0f12299
Compare
I don't understand the commit validation error:
It sounds like |
@lavoiesl - I wonder if the underscore is the problem. Could you update to a hyphen? |
0f12299
to
432007d
Compare
The tooling does support library scopes. Remove the scope |
432007d
to
7dd5a1e
Compare
|
7dd5a1e
to
30b7845
Compare
Yes sorry. It is indeed the title. When we merge PRs we squash all commits down into a single commit using the PR title. |
@lavoiesl - Thanks for this PR! I'd like to get input from other members of the SIG about this change. I've added it to the agenda for tomorrow's meeting. You're welcome to join the SIG meeting if you're interested! You can access the calendar invite by joining this Google Group. We meet Tuesdays at 9:00 AM PT. Mostly, I'm concerned about setting a precedent to accommodate dynamic configuration. I'm not sure what OTel's stance is on this idea and want more input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lavoiesl, we chatted about this in the SIG and are comfortable merging in this change.
Though we don't condone nor support dynamic changes in configuration, we can make this adjustment to resolve the regression you're experiencing.
Thank you for your submission and for adding tests too :)
@lavoiesl - It looks like the branch is out-of-date with |
30b7845
to
7305fa3
Compare
Awesome, thank you! Updated 🚀 |
Fixes #1077 as introduced at #677 (comment)
Added a unit test for good measure.