-
Notifications
You must be signed in to change notification settings - Fork 624
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
Rename MetricRecord to ExportRecord to comply with Metrics Spec #1367
Rename MetricRecord to ExportRecord to comply with Metrics Spec #1367
Conversation
d126686
to
6b32cee
Compare
Docs are automatically updated from the doc strings and class names should be reflected automatically. |
@@ -26,7 +26,7 @@ | |||
|
|||
from opentelemetry import metrics | |||
from opentelemetry.sdk.metrics import MeterProvider | |||
from opentelemetry.sdk.metrics.export import ConsoleMetricsExporter | |||
from opentelemetry.sdk.metrics.export import ConsoleExporter |
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.
Any reason why we are renaming the exporters?
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.
I don't think this name is great. What if I am exporting both traces and metrics to console? It is confusing as to which one I am using without the "Metrics" prefix.
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.
I wasn't sure about renaming everything either. I thought I would rename everything that could possibly renamed to be consistent with MetricRecord being changed to ExportRecord, get feedback and remove what might be unnecessary or wrong. You bring a valid point that if metric exporters and tracing exporters at once, the user would have to use import ... as MetricsExporter which isn't ideal. Would it be fine to only rename the interface from MetricsExporter to Exporter but leave the exporters themselves as PrometheusMetricsExporter, OTLPMetricsExporter, etc. ?
It seemed weird that some of the classes in the metric/init.py file had Metric in the name while others did not. On the Go side and spec, the metrics exporter is always just referred to as exporter although I personally find MetricsExporter to be more explicit.
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.
I do not like referring to the Go implementation as the source of truth. Rather we should follow what the specs are saying. With that being said, naming has been an issue since day one and from a usage standpoint, just "Exporter" is not super clear to me as a user (maybe in the context of metrics it makes sense).
Would it be fine to only rename the interface from MetricsExporter to Exporter but leave the exporters themselves as PrometheusMetricsExporter, OTLPMetricsExporter, etc. ?
I don't have a strong opinion on what class names should be that are NOT exposed to the user. I think the only concern for this is for users that are extending and creating their own metrics exporters, the confusing point above still stands.
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.
The spec calls the component Exporter rather than MetricsExporter which makes sense since the other components of the export pipeline are called Accumulator/Processor rather than MetricsAccumulator/MetricsProcessor. I guess the reason behind specifying Metrics only for the exporter is that there is also an "exporter" component in tracing but not the others. For now, I will narrow down this PR to only renaming MetricRecord to ExportRecord to comply with the spec since that portion seems to cause no harm.
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.
Something to take into consideration is that the specification does not define any ConsoleExporter which means our ConsoleExporter
is not under its specifications, so its name can remain as it is, in my opinion, of course.
Also, we try as much as possible that all the changes in the PRs are described by its name, renaming ConsoleExporter falls out of this description.
That being said, great! Approved 👍
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.
I changed the name after removing the renaming of all the exporters. Console exporter should no longer be part of the PR. Maybe it is bad practice to rename PRs though.
baf3b6e
to
83f24dd
Compare
Once this is merged, the datadog exporter on the opentelemtry-python-contrib will also need to be updated to use ExportRecord instead of MetricRecord. What is the standard procedure for changes on the main repo that have repurcussions on the contrib repo? @lzchen |
3005461
to
00916af
Compare
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.
LGTM, please update the branch and it can be merged
00916af
to
4686555
Compare
Updated branch, should be ready to merge @codeboten. Realized datadog exporter on contrib repo is only for tracing. Only instance of metric record on contrib repo is in my own Prometheus RW exporter PR which I can update myself so no need to link a contrib commit SHA. |
Description
Renaming all instances of MetricRecord to ExportRecord to adhere to Metric Spec
Relates to issues #1345 and #1307
Type of change
How Has This Been Tested?
I ran tox to make sure nothing had been accidentally broken while renaming.
Checklist: