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 logging and metrics #10

Merged
merged 5 commits into from
Mar 14, 2022
Merged

Conversation

carrieedwards
Copy link
Contributor

This PR adds metrics and logging to the influx2cortex proxy.

@carrieedwards carrieedwards marked this pull request as ready for review March 4, 2022 19:34
Copy link
Contributor

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

This looks good! Just some comments about logging levels.

pkg/influx/api.go Outdated Show resolved Hide resolved
pkg/influx/api.go Outdated Show resolved Hide resolved
pkg/influx/api.go Outdated Show resolved Hide resolved
pkg/influx/api.go Outdated Show resolved Hide resolved
@carrieedwards carrieedwards requested a review from ywwg March 8, 2022 16:26
Copy link
Contributor

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

this looks good -- I am not sure how labels get applied, but let's test that.

Approving so as not to block on this small change

expMetrics: `
# HELP influxdb_proxy_ingester_data_conversion_seconds Time (in seconds) spent converting ingested InfluxDB data into Prometheus data.
# TYPE influxdb_proxy_ingester_data_conversion_seconds histogram
influxdb_proxy_ingester_data_conversion_seconds_bucket{le="0.005"} 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to set some labels? I am not sure how that works, but we want to make sure the metrics get tagged with the hostname and cluster, etc. That's normally part of the Registerer I think? Can you make a change in the test so a label gets applied to these metrics so we can confirm that works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirmed that the namespace and cluster labels are added at a later point, so they wouldn't be added for the unit test

@carrieedwards carrieedwards merged commit 235e053 into main Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants