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
Add metadata to push payload #9694
Add metadata to push payload #9694
Changes from 25 commits
b799556
b0810c8
b7ae131
9bec73a
d876c21
9c6fdef
b4ed160
8c6f22e
c854d06
95ba8a2
7db3e54
ea8f3f6
73cf5dc
7b77c69
44b0932
617e864
f43dfab
384d556
4088112
0b0af55
d4e311c
10d404e
fcd1e66
64d3d6c
96c804d
7f25bb8
752ae6b
4b67145
aa58916
4c60a9e
4028758
8c76150
fb505b4
ccaf192
fb42e9c
eaf235c
7438809
f6e6918
f8c33aa
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.
Previously this stored a pointer, now it stores a struct. Not sure we want this (would need to be tested independently for performance)
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 comes from this PR:
#1145
The only reason why I think this is a pointer is so we don't make a copy of the stream when calling NewStream which receives a ptr to
loghttp.Stream
https://github.com/grafana/loki/pull/1145/files#diff-71619e1c80a73b34eade235a55d012a0ddbb3375b8d4ac89c1f4fd672145b915R34
With Sandeep's changes, we no longer use that since now we decode from json to
loghttp.PushRequest
to then cast it tologproto.PushRequest
loki/pkg/util/unmarshal/unmarshal.go
Line 22 in 44b0932
So having that as a ptr or not shouldn't make much difference.
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.
Don't we need to check types for the values here to ensure they're strings?
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.
Done with 384d556
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 scariest thing in the PR ) we should use it only if we check that the buffer is not updated, however, a lot of functions in stack trace mean that somebody can do some optimization to reuse the slice and it will break all our implementation. Why do we need it here?
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.
As discussed in Slack, removed.