Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: change level of TCP events #3285

Merged
merged 2 commits into from
Jul 31, 2020
Merged

chore: change level of TCP events #3285

merged 2 commits into from
Jul 31, 2020

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Jul 31, 2020

Noticed this while worked on #3188

@fanatid
fix log message for TcpEventReceived
41984fc 
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
@fanatid
change level for tcp events
5cb7568 
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
@fanatid fanatid added the domain: traces Anything related to Vectors' trace events label Jul 31, 2020
@fanatid fanatid self-assigned this Jul 31, 2020
@fanatid fanatid mentioned this pull request Jul 31, 2020
Merged
@fanatid
Copy link
Contributor Author

fanatid commented Jul 31, 2020

I'm not sure about message correctness, is this ok: received / sending?

@fanatid fanatid mentioned this pull request Jul 31, 2020
Merged
bruceg
bruceg reviewed Jul 31, 2020
View reviewed changes
src/internal_events/tcp.rs
debug!(message = "sending event.", byte_size = %self.byte_size);
trace!(message = "sending event.", byte_size = %self.byte_size);
Copy link
Member

@bruceg bruceg Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other "sending" messages but one (src/sinks/util/sink.rs) use debug!, so this would be a convention change. I agree trace seems more reasonable, but we should plan to change all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, should we change all "sending" debug! to trace! in one PR?

Copy link
Contributor

@binarylogic binarylogic Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the single concern is this to lower the level of these logs, then one PR should be fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there were two concerns—one typo vs the debug messages that should be—and so the question was to bundle them together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Given these are simple, I think it's ok to bundle them.

src/internal_events/tcp.rs
debug!(message = "sending event.", byte_size = %self.byte_size);
trace!(message = "received event.", byte_size = %self.byte_size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…and yet, most of the "received" messages are traces. This obviously should go in for the typo at least.

binarylogic
binarylogic approved these changes Jul 31, 2020
View reviewed changes
Copy link
Contributor

@binarylogic binarylogic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for changing this. I agree that this should be TRACE level. Anything above trace should be rate limited in the hot path.

@binarylogic binarylogic mentioned this pull request Jul 31, 2020
Closed
@fanatid fanatid merged commit bafb7c3 into master Jul 31, 2020
@fanatid fanatid deleted the internal-events-level-change branch July 31, 2020 17:34
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
@fanatid
chore: change level of TCP events (vectordotdev#3285)
e6bb328 
* fix log message for TcpEventReceived

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>

* change level for tcp events

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruceg bruceg bruceg left review comments

@binarylogic binarylogic binarylogic approved these changes

@lukesteensen lukesteensen Awaiting requested review from lukesteensen

Assignees

@fanatid fanatid

Labels
domain: traces Anything related to Vectors' trace events
Projects
None yet
Milestone
2020.07.20 - The Great Orientation
Development

Successfully merging this pull request may close these issues.
3 participants
@fanatid @bruceg @binarylogic