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
feat: support http_request field #120
feat: support http_request field #120
Changes from 13 commits
55383da
3c67803
4be2e19
61115d1
f7aa0e0
1459f7e
96cf896
a8d54c8
841e68a
a3bd27c
e8733f4
2f73049
e6ccd30
b653b22
6ca954e
16e7888
36a71ab
6b549bd
e91548c
5f9d932
69c0d39
5bb9e51
6d1974d
f0f5334
fe9104c
95ceb66
399cbf7
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.
Future edge case to take care of:
Given user input is
X-Cloud-Trace-Context: TRACE_ID/SPAN_ID;o=TRACE_TRUE
where any of the values can be nil or 0. This logic to split by / and grab the first value might give youspan id
instead.Seems super niche edge case, but I actually got a similar ticket for this in Go.
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.
Hmm I'm not sure I understand. Can you have a span without a trace? Do you have a ticket or doc I can look over?
Either way, I'm not a huge fan of this parsing logic. I'm hoping to fix it up when I add support for spans soon
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 shouldn't be possible to have a trace as null but span as non-null. Curious how this was a case in Go?
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.
The edge cases I'm referring to here is just that the
X-Cloud-Trace-Context
field is provided by the user.So the user could manually write anything here. e.g.
'/hello/world'
and we'd grep the first token as trace_id. Adding better regex later on could help.This was the original Go issue.
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.
Where we tell users they can manually write this header: https://cloud.google.com/trace/docs/setup#force-trace
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.
Hmm good to keep in mind, yeah it might be helpful to throw an exception or something if the header doesn't match the expected format
I'll probably fix that in a later PR when I add support for spans though. That's when proper parsing will be more important. For now, this is the logic that was already in place
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 see you're using
http_pb2_request
does that work properly with theLogEntry
?python-logging/google/cloud/logging_v2/entries.py
Line 173 in 6349b89
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 it should. This is the type used by the underlying LogEntry proto, so I assumed it would be preferred:
python-logging/google/cloud/logging_v2/types/log_entry.py
Line 192 in ff90fd2
But I'm still getting used to working with gapic code. Do you prefer to just use dictionaries for these kinds of fields?