Skip to content

Conversation

@pracucci
Copy link
Contributor

What this PR does:
While working on the metrics registration refactoring I've realized the current integration/metrics_test.go is not that good because doesn't exercise much Cortex code.

In this PR I've removed the metrics-specific test and I've added the same assertions to all other integration tests, so that more Cortex code is exercised (ie. chunks storage, blocks storage, caching, ...).

Which issue(s) this PR fixes:
N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pracucci pracucci force-pushed the improve-metrics-integration-tests branch from 4a2667f to 95c7344 Compare March 20, 2020 11:30
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM, one quick question

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused where this is coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no such metric prefix. I just wanna avoid we'll add it in the future. I think it's a trivial mistake to do.

For example in the past we did a similar mistake with the query frontend and now we have mixed prefixes cortex_query_frontend and cortex_frontend.

Makes sense to you? If it's just my paranoia I can remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine to leave it IMO

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the improve-metrics-integration-tests branch from 93c54e2 to ad2a6aa Compare March 20, 2020 16:26
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci merged commit a68f440 into cortexproject:master Mar 20, 2020
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.

2 participants