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
Beat [2/4]: implement
blockbeat
#8894Beat [2/4]: implement
blockbeat
#8894Changes from all commits
0f8a524
e263ece
373b795
797c10e
5d91b5e
269e37c
09ad818
6a6872a
eed75c7
627065c
bb494da
2c153b5
6daea9c
48dd2ea
197f1e8
07872fc
8bd6153
b63855d
9ab2e53
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.
I wonder if we can use the advantages of structured logging here already @ellemouton ?
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.
discussed offline and the conclusion here was that we should do this as a follow up just to avoid all the rebase conflicts
(although.. @yyforyongyu - do the follow-ups actually touch this main beat logic? cause if not then there might not be any conflicts to worry about)
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.
but yeah - agreed that just having a
height=x
kv pair is nicer at the end of the day. Another reason to pass a context through where we can here.but yeah - if too many merge conflicts, then a follow up is all good
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.
Yeah I also wanna try it - does it mean we don't need the prefix logger anymore? Is it possible we could have something like this being logged?
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 think we should go with structured logging from the beginning, this might also faciliated the structure and we do not need a logger object in the blockbeat.
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 tricky cause we need to do proper context usage first which is hard to contain to just a single system...
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.
yeah we can eventually remove the prefix logger. I think we just need to get used to the
k=v
pairs at the end & of the log line that we can filter on instead of the prefix. Prefix i think should just be the subsystem.