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.

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 elements to the metrics events #1987

Merged
merged 7 commits into from Dec 8, 2021

Conversation

tobim
Copy link
Member

@tobim tobim commented Nov 30, 2021

This change makes it possible to add pairs of strings as metadata to a metrics event.

This also removes code for the defunct server side STDOUT heartbeat and 2 commits adding accountant actor handles to the meta index and partition in preparation of additional metrics.

This depends on #1986.

📝 Checklist

  • The PR description contains instructions for the reviewer, if necessary.
  • Add a changelog entry.
  • Update the docs.

🎯 Review Instructions

By commit.

@tobim tobim requested a review from a team November 30, 2021 12:01
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

I think this is a user-facing change: adapting the metrics layout requires a changelog entry and a documentation update.

Can you also expand on why you are making this change? That's not immediately obvious from the PR description, and I think it really should be.

libvast/vast/system/report.hpp Outdated Show resolved Hide resolved
libvast/vast/system/report.hpp Outdated Show resolved Hide resolved
@tobim tobim force-pushed the story/sc-29855/feature/metrics-metatdata branch 2 times, most recently from 6d6aaa9 to 24bcb22 Compare December 6, 2021 12:20
Base automatically changed from story/ch26781/query-id to master December 6, 2021 13:16
tobim and others added 6 commits December 8, 2021 14:39
The periodic throughput status message to STDOUT has been broken
since we switched to spdlog for logging. We never heard a complaint
about that so it's save to remove that logic now.

The status command can be used as an alternative way to make sure
the system is operating correctly.
This change makes it possible to add pairs of strings as metatdata
to a metrics event.
Co-authored-by: Dominik Lohmann <mail@dominiklohmann.de>
@tobim tobim force-pushed the story/sc-29855/feature/metrics-metatdata branch from 24bcb22 to 1e780de Compare December 8, 2021 13:52
Copy link
Contributor

@6yozo 6yozo left a comment

Choose a reason for hiding this comment

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

We did the review synchronously, paired up with Tobias.

Good to go.

@tobim tobim force-pushed the story/sc-29855/feature/metrics-metatdata branch from 1e780de to 03f68df Compare December 8, 2021 15:04
@tobim
Copy link
Member Author

tobim commented Dec 8, 2021

I had to push once more to fix the formatting in the changelog message.

@tobim tobim enabled auto-merge December 8, 2021 15:04
@tobim tobim merged commit c272ac7 into master Dec 8, 2021
@tobim tobim deleted the story/sc-29855/feature/metrics-metatdata branch December 8, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants