Metrics Console Exporter + Add last_updated_timestamp to metrics#192
Metrics Console Exporter + Add last_updated_timestamp to metrics#192lzchen merged 23 commits intoopen-telemetry:masterfrom
Conversation
lzchen
requested review from
Oberon00,
a-feld,
c24t,
carlosalberto,
reyang and
toumorokoshi
as code owners
|
[WIP] PR is a placeholder until export logic is spec'd out. DO NOT MERGE. |
Are you still looking for comments here? You can mark the PR as draft if not. |
toumorokoshi
left a comment
toumorokoshi
left a comment
There was a problem hiding this comment.
I'd prefer to see unit tests, but can be convinced to approve if it's blocking other efforts.
| """ | ||
|
|
||
|
|
||
| class ConsoleMetricsExporter(MetricsExporter): |
There was a problem hiding this comment.
I'd vote for the ability to add a format string here, similar to logging. e.g. I may want to emit things as JSON on the console.
There was a problem hiding this comment.
I'd want to be consistent with the ConsoleSpanExporter for now.
There was a problem hiding this comment.
okay. Random question, maybe for you and @Oberon00: any issues if I do add the ability to format output to both of these in a followup PR?
There was a problem hiding this comment.
I don't see a problem with that. Go for it!
| ) -> "MetricsExportResult": | ||
| for metric_tuple in metric_tuples: | ||
| handle = metric_tuple[0].get_handle(metric_tuple[1]) | ||
| print( |
There was a problem hiding this comment.
can we add a way to pass the stream in the init? that way I could also choose to log in other streams, such as stderr.
There was a problem hiding this comment.
Same thought as above: any issue with me adding this flexibility in a followup PR?
|
Follows SDK spec |
|
Reopening this. We can have the ConsoleExporter exist in the package with the simple |
c24t
left a comment
c24t
left a comment
There was a problem hiding this comment.
One blocking comment about the test, looks good otherwise!
opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/__init__.py
Outdated
Show resolved
Hide resolved
| from opentelemetry.sdk.metrics import Counter, Meter | ||
| from opentelemetry.sdk.metrics.export import ConsoleMetricsExporter | ||
|
|
||
| metrics.set_preferred_meter_implementation(lambda T: Meter()) |
There was a problem hiding this comment.
would be great if the SDK meter implementation didn't need this additional lambda. Feel a little weird that this pattern will have to be added in every codebase that wants to use the standard meter implementation.
There was a problem hiding this comment.
Right. I think as part of [#195], there will be an easier way to initialize these implementations (perhaps a default implementation is none is specified).
| from enum import Enum | ||
| from typing import Sequence, Tuple | ||
|
|
||
| from .. import Metric |
There was a problem hiding this comment.
random aside, but it feels weird to have to import from a parent module like this.
Maybe a style thing but I prefer importing from the submodule it is defined in:
from ..metric import Metric
prevents someone adding a cyclic dependency by importing a value in here into the top level module as well.
| """ | ||
|
|
||
|
|
||
| class ConsoleMetricsExporter(MetricsExporter): |
There was a problem hiding this comment.
okay. Random question, maybe for you and @Oberon00: any issues if I do add the ability to format output to both of these in a followup PR?
| ) -> "MetricsExportResult": | ||
| for metric_tuple in metric_tuples: | ||
| handle = metric_tuple[0].get_handle(metric_tuple[1]) | ||
| print( |
There was a problem hiding this comment.
Same thought as above: any issue with me adding this flexibility in a followup PR?
| from opentelemetry.sdk.metrics.export import ConsoleMetricsExporter | ||
|
|
||
|
|
||
| class TestConsoleMetricsExporter(unittest.TestCase): |
There was a problem hiding this comment.
since we've made the decision to use pytest now, we no longer need to inherit from unittest.TestCase and can just use the general assert. I guess after #128 gets merged in we could discuss refactoring these.
* feat(zipkin-exporter): implement * fix(zipkin): use time fns * refactor(exporter-zipkin): linter fix
3 participants
Implemented a console exporter for metrics, which simply prints metrics data to the console.
As well, added last_updated_timestamp field to metrics, certain exporters (Azure) need this field to be tracked.