Sanitize spans with null process or empty service name #3631
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #3631 +/- ##
==========================================
+ Coverage 96.49% 96.53% +0.03%
==========================================
Files 266 267 +1
Lines 15622 15653 +31
==========================================
+ Hits 15075 15111 +36
+ Misses 457 453 -4
+ Partials 90 89 -1
Continue to review full report at Codecov.
|
|
@codecov[bot] is not allowed to run commands |
yurishkuro
force-pushed
the
null-process
branch
from
97bc6f9
to
2a468b8
Compare
|
@albertteoh can you review, please? |
albertteoh
reviewed
May 5, 2022
| "github.com/jaegertracing/jaeger/model" | ||
| ) | ||
|
|
||
| var ( |
cmd/collector/app/span_processor.go
Outdated
| @@ -96,13 +96,18 @@ func newSpanProcessor(spanWriter spanstore.Writer, additional []ProcessSpan, opt | |||
| } | |||
| boundedQueue := queue.NewBoundedQueue(options.queueSize, droppedItemHandler) | |||
|
|
|||
| sanitizers := append([]sanitizer.SanitizeSpan{}, sanitizer.StandardSanitizers...) | |||
There was a problem hiding this comment.
sanitizer.StandardSanitizers is a globally accessible slice that can be mutated. What do you think about unexporting sanitizer.StandardSanitizers and adding a sanitizer.GetStandardSanitizers() func?
Then (I don't think we need to append to an empty slice?):
sanitizers := sanitizer.GetStandardSanitizers()
|
@yurishkuro, apologies for the late review, I've been away for a couple of weeks. |
yurishkuro
commented
May 10, 2022
| @@ -68,7 +69,7 @@ wait_for_it() { | |||
| ''%{http_code}'' | |||
| ) | |||
| local counter=0 | |||
| while [[ "$(curl ${params[@]} ${url})" != "200" && ${counter} -le 30 ]]; do | |||
| while [[ "$(curl ${params[@]} ${url})" != "200" && ${counter} -le 60 ]]; do | |||
There was a problem hiding this comment.
1min (2*30) seems insufficient sometimes, we can wait longer if it takes OS longer to start
albertteoh
previously approved these changes
May 10, 2022
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
albertteoh
pushed a commit
to albertteoh/jaeger
that referenced
this pull request
Jul 13, 2022
…#3631) * Sanitize spans with null process or empty service name Signed-off-by: Yuri Shkuro <github@ysh.us> * cleanup Signed-off-by: Yuri Shkuro <github@ysh.us> * review comments Signed-off-by: Yuri Shkuro <github@ysh.us> * wait longer Signed-off-by: Yuri Shkuro <github@ysh.us> * add tests Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Sanitize spans with empty service name or null process. This should protect the pipeline from malformed spans submitted externally, as reported in #3578. It will not protect against malformed spans written directly to Kafka - we could do that too, but it changes the expectations of the deployment that Kafka is a public API into ingester, which would be a new guarantee and will require ingester to run preprocessing logic similar to collector.