-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[statsdreceiver] add test for empty statsd tags #32336
[statsdreceiver] add test for empty statsd tags #32336
Conversation
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
9088c4f
to
5defde4
Compare
@atoulme is there any interest in this bugfix? |
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.
looks simple enough, and guarding against input seems like a good idea.
@diurnalist take a look at the conflict and see if your test passes with latest main without your modification. |
we see some payloads with completely empty tags, which we can safely ignore. there is already support for this in the receiver; this simply adds a test to track future regressions on this behavior.
5defde4
to
989265c
Compare
@atoulme yeah I just noticed that a change 4 days ago obviated the need for this :p - i guess this PR's only contribution then is the test itself, I think the other PR fixed the panic issue as a side-effect :) |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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.
More tests sound good thanks!
.chloggen/b_statsd-empty-tags.yaml
Outdated
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [user] |
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 don't need a changelog for test. Please remove this
Description: we see some payloads with completely empty tags, which we can safely ignore. This is already handled by the receiver, but we did not have a test for it.
Close #32337
Testing: adds simple test to receiver
Documentation: n/a