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

[lightprometheusreceiver] initial working draft #2921

Merged
merged 8 commits into from
Apr 6, 2023

Conversation

dloucasfx
Copy link
Contributor

@dloucasfx dloucasfx commented Apr 5, 2023

This is the initial working draft for lightprometheusreceiver

@dloucasfx dloucasfx requested review from a team as code owners April 5, 2023 00:20
@atoulme atoulme requested a review from a team as a code owner April 5, 2023 00:42
@atoulme atoulme force-pushed the lightprometheusreceiver branch 2 times, most recently from d1c26b5 to d457e48 Compare April 5, 2023 04:28
@rmfitzpatrick
Copy link
Contributor

Would it be better to update the simple prometheus receiver that we already contributed instead of adding a new one? With prometheus, simple prometheus and the prometheus-exporter monitor this would be the fourth prometheus receiver for scraping metric views.

@rmfitzpatrick
Copy link
Contributor

rmfitzpatrick commented Apr 5, 2023

Please add an integration test. There's an existing one w/ internal metrics that could be used as a basis: https://github.com/signalfx/splunk-otel-collector/blob/main/tests/receivers/prometheus/internal_prometheus_test.go

Sorry, not sure how I missed that.

Copy link
Contributor

@hughesjj hughesjj left a comment

Choose a reason for hiding this comment

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

two nits but honestly LGTM otherwise, especially for a draft or triage.

}
}
}
case dto.MetricType_HISTOGRAM, dto.MetricType_GAUGE_HISTOGRAM:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we consider adding an option to this receiver to drop certain type of prom metrics?
I have a use case where I want to collect prom metrics from some k8s components but am not really interested in the histogram metrics because the buckets are generated dynamically and w/o native UI support for histogram in O11y, visualizing these is not a good experience. So I would just like to not collect them instead of adding exclusion rules for specific metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, yes we can and I think we should. @atoulme wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

The usual guidance is to drop the metrics later in a processor filter. I think the signalfx exporter also translates histograms to a different metric type.
We can probably filter at the source though, while we're in here.

Copy link
Contributor Author

@dloucasfx dloucasfx Apr 5, 2023

Choose a reason for hiding this comment

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

right, filtering at the source will save us a few bytes... anything we can do to save one bit of memory at a time :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

let's do it in a separate PR. Less is more.

Copy link
Contributor

@atoulme atoulme Apr 12, 2023

Choose a reason for hiding this comment

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

OTL-2009 is filed to follow up.

@atoulme
Copy link
Contributor

atoulme commented Apr 6, 2023

Would it be better to update the simple prometheus receiver that we already contributed instead of adding a new one?

True, the aim is to test it here and move it upstream, and see if we can use simpleprometheusreceiver. Tough though as it changes the config.

@atoulme atoulme merged commit 790e5dc into signalfx:main Apr 6, 2023
@hughesjj
Copy link
Contributor

... Huh, I wonder if we can version Config schemas similar to how we version metrics schemas in otel. Not the first time I've seen such an ask.

that said additive config changes are usually pretty easy.

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.

5 participants