-
Notifications
You must be signed in to change notification settings - Fork 652
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
Metrics Console Exporter + Add last_updated_timestamp to metrics #192
Changes from 20 commits
368d4d5
12d73f9
b2af66c
400fa99
2090ea7
0377559
214d7a0
062d267
736b2b7
501d515
b217627
e893947
352d807
3c1b6e7
820ae24
4d1f5b9
185ae6a
80e80ab
3458703
7501c34
f029fb5
a1762b4
b6be47d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
lzchen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
from enum import Enum | ||
from typing import Sequence, Tuple | ||
|
||
from .. import Metric | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
prevents someone adding a cyclic dependency by importing a value in here into the top level module as well. |
||
|
||
|
||
class MetricsExportResult(Enum): | ||
SUCCESS = 0 | ||
FAILED_RETRYABLE = 1 | ||
FAILED_NOT_RETRYABLE = 2 | ||
|
||
|
||
class MetricsExporter: | ||
"""Interface for exporting metrics. | ||
|
||
Interface to be implemented by services that want to export recorded | ||
metrics in its own format. | ||
""" | ||
|
||
def export( | ||
self, metric_tuples: Sequence[Tuple[Metric, Sequence[str]]] | ||
) -> "MetricsExportResult": | ||
"""Exports a batch of telemetry data. | ||
|
||
Args: | ||
metric_tuples: A sequence of metric pairs. A metric pair consists | ||
of a `Metric` and a sequence of strings. The sequence of | ||
strings will be used to get the corresponding `MetricHandle` | ||
from the `Metric` to export. | ||
|
||
Returns: | ||
The result of the export | ||
""" | ||
|
||
def shutdown(self) -> None: | ||
"""Shuts down the exporter. | ||
|
||
Called when the SDK is shut down. | ||
""" | ||
|
||
|
||
class ConsoleMetricsExporter(MetricsExporter): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd want to be consistent with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a problem with that. Go for it! |
||
"""Implementation of :class:`MetricsExporter` that prints metric handles | ||
lzchen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
to the console. | ||
|
||
This class can be used for diagnostic purposes. It prints the exported | ||
metric handles to the console STDOUT. | ||
""" | ||
|
||
def export( | ||
self, metric_tuples: Sequence[Tuple[Metric, Sequence[str]]] | ||
) -> "MetricsExportResult": | ||
for metric, label_values in metric_tuples: | ||
handle = metric.get_handle(label_values) | ||
print( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thought as above: any issue with me adding this flexibility in a followup PR? |
||
'{}(data="{}", label_values="{}", metric_data={})'.format( | ||
type(self).__name__, metric, label_values, handle | ||
) | ||
) | ||
return MetricsExportResult.SUCCESS |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import io | ||
import sys | ||
import unittest | ||
|
||
from opentelemetry.sdk import metrics | ||
from opentelemetry.sdk.metrics.export import ConsoleMetricsExporter | ||
|
||
|
||
class TestConsoleMetricsExporter(unittest.TestCase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for adding tests! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
def test_export(self): | ||
exporter = ConsoleMetricsExporter() | ||
metric = metrics.Counter( | ||
"available memory", | ||
"available memory", | ||
"bytes", | ||
int, | ||
("environment",), | ||
) | ||
label_values = ("staging",) | ||
handle = metric.get_handle(label_values) | ||
result = '{}(data="{}", label_values="{}", metric_data={})\n'.format( | ||
ConsoleMetricsExporter.__name__, metric, label_values, handle | ||
) | ||
output = io.StringIO() | ||
sys.stdout = output | ||
exporter.export([(metric, label_values)]) | ||
sys.stdout = sys.__stdout__ | ||
self.assertEqual(len(output.getvalue()), len(result)) | ||
lzchen marked this conversation as resolved.
Show resolved
Hide resolved
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).