-
Notifications
You must be signed in to change notification settings - Fork 103
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
Support gcp.source_location being set as other types #794
Support gcp.source_location being set as other types #794
Conversation
By copying the logic from parsing gcp.http_request, this PR adds support to also parse `gcp.source_location` from types other than Bytes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #794 +/- ##
==========================================
+ Coverage 70.02% 70.30% +0.28%
==========================================
Files 42 42
Lines 4877 4886 +9
==========================================
+ Hits 3415 3435 +20
+ Misses 1311 1303 -8
+ Partials 151 148 -3 ☔ View full report in Codecov by Sentry. |
/gcbrun |
To make codecov happy :)
To make codecov EVEN MORE happy :)
exporter/collector/logs_test.go
Outdated
@@ -396,6 +395,103 @@ func TestLogMapping(t *testing.T) { | |||
}, | |||
maxEntrySize: defaultMaxEntrySize, | |||
}, | |||
{ | |||
name: "log with bad source location (bytes)", |
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.
What makes this test "bad"?
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.
It's because the value provided for gcp.source_location
can't actually be parsed, the json Unmarshal fails due to an invalid type in one of the fields. Is there a nicer way I should say that concisely in the test or should I add a comment?
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.
"log with wrong type in sourceLocation (bytes)"
, perhaps?
* Renamed `bytes` to `valueBytes` to avoid import shadow * Moved `expectedError` to top of the test case struct to handle field alignment error
/gcbrun |
@quentinmit i'll wait for your approval to merge |
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.
See comment, but otherwise this LGTM
/gcbrun |
1 similar comment
/gcbrun |
Let's see if the e2e tests will work today 🙏 |
/gcbrun |
I still don't seem to have permission to run these :( |
/gcbrun |
2 similar comments
/gcbrun |
/gcbrun |
By copying the logic from parsing gcp.http_request, this PR adds support to also parse
gcp.source_location
from types other than Bytes.This fixes the basic case of #792 such that the collector does not panic when setting
gcp.source_location
as something other than bytes.