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 target metadata API #4438

Closed
wants to merge 7 commits into from
Closed

Conversation

philipgough
Copy link
Contributor

@philipgough philipgough commented Jul 12, 2021

This PR implements the target metadata API on the query component and fixes #3870.

The implementation largely follows the example set by the metric metadata PR in #3686 and in fact piggybacks on the service defined in the metadata proto that was introduced in that feature, since the two metadata APIs are closely related.

This avoids the need to add an additional flag to define metadata stores. This makes it more convenient but maybe less desirable (we could not take them out of experimental mode independently for example) and I am happy to introduce an additional flag and rpc service if required.

There is an open question on #3870 but I have followed @ianbillett suggestion and went with (Target, Metric) tuple metadata which essentially provides always on deduplication of metadata (a metric that is duplicated across targets will only be returned once from the API).

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

I have added e2e tests

Signed-off-by: Philip Gough <philip.p.gough@gmail.com>
Signed-off-by: Philip Gough <philip.p.gough@gmail.com>
Signed-off-by: Philip Gough <philip.p.gough@gmail.com>
Signed-off-by: Philip Gough <philip.p.gough@gmail.com>
Signed-off-by: Philip Gough <philip.p.gough@gmail.com>
Signed-off-by: Philip Gough <philip.p.gough@gmail.com>
Copy link
Contributor

@bill3tt bill3tt left a comment

Choose a reason for hiding this comment

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

Woof - there are a huge amount of changes in here!

At a high level scanning the code, I think the changes you are making are the right ones, but this PR is currently too much of a beast for anyone to review thoroughly.

I will ask that you try and split this PR up into smaller chunks that are easier for reviewers to review - that way you are much more likely to have someone review them thoroughly.

Perhaps adding the proto definition and the generated code first (that accounts for most of the added lines), but leave them unused - then come back and raise a PR that actually uses this code?

@philipgough
Copy link
Contributor Author

@ianbillett thanks for the feedback. I have followed the format of #3686 and felt that the commits are fairly independently reviewable but totally understand the size of the PR is not ideal for reviewing.

Will be happy to split the proto and generated code into an initial PR if that is preferable.

Signed-off-by: Philip Gough <philip.p.gough@gmail.com>
@stale
Copy link

stale bot commented Sep 19, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Sep 19, 2021
@stale stale bot closed this Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement target metadata API
2 participants