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
Remote Sampling - XRay (Part 1) (Fetching Sampling Rules) #1536
Remote Sampling - XRay (Part 1) (Fetching Sampling Rules) #1536
Changes from 45 commits
dcc01de
1dd54d9
03f6fb4
5919be5
ebd45b4
e1ff7d0
041d9b8
e12a4b1
e906d2a
86c04d1
047f5d0
21db8fa
5204d27
c9c1bca
c956d4b
12c6c74
e042a6f
1f6b745
ada0137
9c6a430
79d6d15
e21f9a6
2e0f29a
7aedf46
741a285
839cff9
ab3d7b0
17d24f3
cdc58fa
d5902d9
36fa11f
c4ed209
f423e01
5224286
9fe987d
70eb25f
8e1f48f
c20df37
fdbebb0
09475dc
eec96d0
43a2ae7
bd59636
d094d58
01a8995
7b1987e
bb2fb94
92740aa
86c5aec
3dea789
30e6c79
ee415a7
c923616
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.
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.
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.
This should probably check if the address already has a scheme. If that should be strictly forbidden, then there should be some validation closer to the user I guess.
Note that while not the first version, eventually this sampler needs to be able to support more configuration, for example TLS or auth headers, as users don't necessarily run collectors as plaintext sidecars.
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.
Yes I think we strictly expect string like
"127.0.0.1:8080"
but yeah you're right I will add some validation around this to double sure.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.
newClient
needs to returnerror
this is a fatal issueThere 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.
Nit
c
orcl
orclient
seems more appropriate thanp
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.
Store
p.endpoint.String()+/GetSamplingRules
(and targets later) in the struct rather than the endpoint to avoid recomputing this every timeThere 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.
umm not sure about that because then we would have to store 2 string fields in a struct for
getSamplingRules
andgetSamplingTargets
. I think usually endpoint gets computed and path would be a string concat operation.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.
This is a struct that won't be initialized many times so having 2 fields is basically no overhead, while computing a string on every poll is overhead.
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.
This seems to be passing a pointer to a pointer - I'm surprised this seems to actually work , but think you don't need
&
, alternatively you might not need to return a pointer here since it should get moved to the callerThere 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 tried removing
&
and it throwed the unmarshal error.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 believe json.Unmarshal() handles this correctly, but it would be nicer to see
i.e., drop the
*
, not the&