-
Notifications
You must be signed in to change notification settings - Fork 7
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 otlp exporters #89
Conversation
actualwitch
commented
Sep 19, 2023
•
edited
Loading
edited
- init function is now mandatory
- prometheus-client exposition is merged into a unified api inside init
- otlp exporters are added as extra packages
- test isolation! Pytest should clear collected metrics after each test runs #36
53af69b
to
5c61050
Compare
47f5f2e
to
664cb10
Compare
when you have a chance could you document how to run the otlp example somewhere? |
src/autometrics/settings.py
Outdated
if ["otlp-proto-http", "otlp-proto-grpc"].count(exporter_options["type"]) > 0: | ||
return OTLPExporterOptions( | ||
type=exporter_options["type"], | ||
endpoint=exporter_options["endpoint"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this (and other configuration lines) use safe accessors + have a default? i don't think it's guaranteed to be defined
alternatively, we could merge it with a default dict, to avoid fallback madness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of these are marked as Optional so it's ok if they are just forwarded as None's. i'm pretty sure they all also have defaults/not requred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i see what's happening - i had to add the whole get_exporter_settings function to make it type typeddicts correctly but this now requires all fields to be filled. i'll look into refactoring this in a more sane way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right on, your latest changes fixed it! as far as i can tell, the option names are the same between our library and the otel one for the otlp exporter, so it might be good to drop a link to their documentation somewhere for available options? if any thing just for our future selves
examples/otlp/start.py
Outdated
from autometrics import autometrics, init | ||
|
||
init( | ||
exporter={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i needed to fill in all fields like this to get things working (related to my comment about providing defaults for the exporter config):
exporter={
"type": "otlp-proto-http",
"endpoint": "http://localhost:4318/v1/metrics",
"insecure": True,
"headers": None,
"credentials": None,
"push_interval": 1000,
"timeout": 1000,
"preferred_temporality": None,
},
962dab6
to
3bf59ba
Compare
I resolved the typing issues by using pydantic for runtime type checking, and added some examples for exporters. also, bumped our fastapi example |
a850213
to
67482f2
Compare
67482f2
to
abf6be6
Compare