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

Update istio mapped metrics #1993

Merged
merged 6 commits into from
Aug 15, 2018
Merged

Update istio mapped metrics #1993

merged 6 commits into from
Aug 15, 2018

Conversation

bobbytables
Copy link
Contributor

@bobbytables bobbytables commented Aug 3, 2018

Updates mapped metrics to use the new names in the templated Prometheus handler in Istio 1.0.

What does this PR do?

Istio updated their Prometheus handler config in 1.0 that changes the names of the metrics. This project is using the older defaulted names from 0.8 and when upgrading to Istio 1.0 this breaks.

Obviously this change is not backwards compatible with 0.8, but moving forward this change helps not go down the rabbit hole that we just went down.

Here is where the metric name changed: https://github.com/istio/istio/blob/master/install/kubernetes/helm/istio/charts/mixer/templates/config.yaml#L455

The change was made in this commit: istio/istio@f883fbf

Motivation

Our graphs broke in DataDog for Istio related metrics when upgrading to Istio 1.0

Updates mapped metrics to use the new names in the templated Prometheus handler in Istio 1.0.
@bobbytables bobbytables requested a review from a team as a code owner August 3, 2018 17:34
@gmmeyer
Copy link
Contributor

gmmeyer commented Aug 3, 2018

Hey @bobbytables! Thanks a lot for your contribution! This PR is pretty good but we need to remain compatible with prior versions of istio. Can you update the map to include both? Can you also update the tests so that it tests v1.0 as well as the prior version?

@bobbytables
Copy link
Contributor Author

Hey @bobbytables! Thanks a lot for your contribution! This PR is pretty good but we need to remain compatible with prior versions of istio. Can you update the map to include both? Can you also update the tests so that it tests v1.0 as well as the prior version?

Yeah I was being lazy 😛 I'll add those right now.

@bobbytables
Copy link
Contributor Author

bobbytables commented Aug 6, 2018

Tbh I don't write python and not sure where to start to support both. It's mostly the pytest that is messing with me and don't quite have time to learn..

@shraykay
Copy link
Contributor

shraykay commented Aug 6, 2018

I'll assist! (bobby's coworker)

@gmmeyer
Copy link
Contributor

gmmeyer commented Aug 6, 2018

Thank you! 😄

If you have any questions I'm happy to help!

Updates mapped metrics to use the new names in the templated Prometheus handler in Istio 1.0.
@gmmeyer
Copy link
Contributor

gmmeyer commented Aug 14, 2018

Hey @bobbytables @shraykay!

Thanks again for this contribution, it's really great. We were about to investigate whether there were metric changes with 1.0 when you submitted this.

This is something we wanna get out asap. If you want any help on it we're very happy to help. If you're on our public slack you can reach out to me there (greg.meyer) and I can help you. Otherwise, you can join our public slack.

If you'd like us to take over this Pull Request, I am also happy to do that.

@shraykay
Copy link
Contributor

Will take a stab at this today, sorry for the delay!

@bobbytables
Copy link
Contributor Author

@gmmeyer @shraykay did the rest.

Copy link
Contributor

@gmmeyer gmmeyer left a comment

Choose a reason for hiding this comment

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

looks good to me! Thank you both very much!!

@gmmeyer gmmeyer merged commit 313f65e into DataDog:master Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants