Skip to content

Conversation

@ross-weir
Copy link
Member

Description

Summary of the changes...

  • I have updated CHANGELOG.md with a new entry if my PR makes any breaking changes or fixes a bug. If my PR removes a feature or changes its behavior, I provide help for users on how to migrate to the new behavior.
  • I have carefully reviewed all my Cargo.toml changes before opening the PRs. (Are all new dependencies necessary? Is any module dependency leaked into the full-node (hint: it shouldn't)?)

Linked Issues

  • Fixes # (issue, if applicable)
  • Related to # (issue)

Testing

Describe how these changes were tested. If you've added new features, have you added unit tests?

Docs

Describe where this code is documented. If it changes a documented interface, have the docs been updated?

Rendered docs are available at https://sovlabs-ci-rustdoc-artifacts.us-east-1.amazonaws.com/<BRANCH_NAME>/index.html


tracing::trace!(
route_count = route_ids.len(),
"Emitting rate limiter metrics at slot {}",
Copy link
Member

Choose a reason for hiding this comment

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

String interpolation will blow up structured loggin, such as loki

Copy link
Member Author

Choose a reason for hiding this comment

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

how?

Copy link
Member Author

Choose a reason for hiding this comment

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

should I use format! instead?

Copy link
Member

Choose a reason for hiding this comment

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

Because it will index body, so instead having single searchable body "Emitting rate limiter metrics" it will have it at which slot, which is increasing.

I would suggest to use it as attribute, same way we have route_count

Copy link
Member Author

Choose a reason for hiding this comment

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

The body will be searchable with Emitting rate limiter metrics.*

It's a high cardinality field, is it OK to add it as an attribute?

Copy link
Member

Choose a reason for hiding this comment

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

It's a high cardinality field, is it OK to add it as an attribute?

Yep, that's the idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants