Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
contrib: fix span start option races #2418
contrib: fix span start option races #2418
Changes from 6 commits
8f24d7b
2ef8ddc
5e8ff05
1b1eba1
260f6d8
9778ffe
f24ebd3
694bf1f
4fe86a7
346aa7c
64fd168
5bc5a22
bcb9b19
ddf67cd
7b0ae77
0b9c2db
bb7a30d
4b0e472
492981b
01dd225
1ab2a4c
3c089f2
d9b7e10
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
While I was reading some contribs changed in this PR, I think we could go further to help thanks to some
ImmutableOptions
wrapper type.For example avoiding:
And doing instead:
Or having a full wrapper type
Options
with anAppend
method so it can do more safety checks?My point is: how can we further improve this API to avoid future mistakes again and maybe add this ability to block any further uses of
append
once we know we are done and anything coming later is in a request handler, from concurrent goroutines.cc @knusbaum
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.
Abstracting the mutations is potentially a good idea, but we would have to be careful to do it in an efficient way so that every
Append
doesn't cause a copy.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.
We should consider it, but let's do so in a follow-up PR. I'd like to keep this PR as minimal as possible given the already large blast radius of changes.
Check failure on line 13 in contrib/internal/options/options.go
GitHub Actions / PR Unit and Integration Tests / lint
Check failure on line 13 in contrib/internal/options/options.go
GitHub Actions / PR Unit and Integration Tests / lint
Check failure on line 36 in contrib/urfave/negroni/negroni.go
GitHub Actions / PR Unit and Integration Tests / lint
Check failure on line 36 in contrib/urfave/negroni/negroni.go
GitHub Actions / PR Unit and Integration Tests / lint
Check failure on line 36 in contrib/urfave/negroni/negroni.go
GitHub Actions / PR Unit and Integration Tests / lint
Check failure on line 36 in contrib/urfave/negroni/negroni.go
GitHub Actions / PR Unit and Integration Tests / lint