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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit fine grained metrics for every part of the query pipeline #1992

Merged
merged 13 commits into from Dec 9, 2021

Conversation

tobim
Copy link
Member

@tobim tobim commented Dec 6, 2021

This change adds metrics events that are emitted for each query from the meta index, partition and store components. All new metrics and the preexisting exporter metrics events use the query ID as metadata value to allow cross-referencing.

馃摑 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

馃幆 Review Instructions

Each commit can be reviewed individually in order.
The last commit modifies the interface of the store plugin, which is unfortunate and I would like to discuss alternative approaches.

@tobim tobim added the feature New functionality label Dec 6, 2021
@tobim tobim requested review from dominiklohmann and a team December 6, 2021 13:09
@dominiklohmann
Copy link
Member

I'm approving of the changes to the plugin interface. We should consider making it a component plugin in the future, though, but this PR is neither the time nor the place to do it.

@tobim tobim force-pushed the story/sc-29855/feature/metrics-metatdata branch 2 times, most recently from 1e780de to 03f68df Compare December 8, 2021 15:04
Base automatically changed from story/sc-29855/feature/metrics-metatdata to master December 8, 2021 15:48
@tobim tobim force-pushed the story/sc-29856/feature/query-pipeline-metrics branch from 4a8eb62 to 1aa2620 Compare December 8, 2021 17:16
@tobim tobim enabled auto-merge December 9, 2021 09:48
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.

One general question: Did you try to set up some dashboards using these metrics? Would be lovely to see a screenshot shared in the community Slack or so.

On some systems `int64_t` is defined as `singed long int`, but
the representation for the duration of `std::chrono::steady_clock`
can be different. This caused a typed mpi mismatch that is hereby
fixed.
@tobim tobim force-pushed the story/sc-29856/feature/query-pipeline-metrics branch from ba9f97b to 87d3acd Compare December 9, 2021 12:53
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, and fixed compilation problems.

Good to go.

@tobim tobim merged commit a3f3ba9 into master Dec 9, 2021
@tobim tobim deleted the story/sc-29856/feature/query-pipeline-metrics branch December 9, 2021 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality
Projects
None yet
3 participants