-
Notifications
You must be signed in to change notification settings - Fork 822
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
feat(metric-reader): add metric-reader #2681
Merged
dyladan
merged 22 commits into
open-telemetry:main
from
dynatrace-oss-contrib:metric-reader
Jan 7, 2022
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
6c4f989
feat(metric-reader): add metric-reader.
pichlermarc 4a767a7
refactor(metric-reader): use callbacks
pichlermarc fd04ff4
refactor(metric-reader): add options so that timeout can be omitted.
pichlermarc df2af4a
refactor(metric-reader): combine TestMetricExporter with WaitingMetri…
pichlermarc 8390e4a
fix(metric-reader): update shutdown and forceFlush usages, fix style.
pichlermarc 78d582d
refactor(metric-reader): add default implementation for onInitialized…
pichlermarc 381c8a0
refactor(metric-reader): use promise pattern instead of callbacks
pichlermarc f9ec116
refactor(metric-reader): update metric collector to use the promise p…
pichlermarc fcfe6b7
fix(metric-reader): pass function instead of Promise to assert.reject…
pichlermarc 6d2690b
fix(metric-reader): do not collect and export before force-flushing t…
pichlermarc 2346295
refactor(metric-reader): remove unused ReaderResult
pichlermarc 59697cf
fix(metric-reader): do not throw on multiple shutdown calls.
pichlermarc 5fa2f39
refactor(metric-reader): move callWithTimeout and TimeoutError to src…
pichlermarc 5081c9c
docs(metric-reader): add link to describe why the prototype is set ma…
pichlermarc 3158422
fix(metric-reader): fix switched-out reader and force-flush options.
pichlermarc 282a75d
fix(metric-reader): do not use default timeouts.
pichlermarc 9a4b24b
fix(metric-reader): make options argument optional, cleanup.
pichlermarc 0ff2c92
fix(metric-reader): actually add batch to _batches in TestMetricExporter
pichlermarc 471d5d5
fix(metric-reader): add test-case for timed-out export.
pichlermarc a3b6796
docs(metric-reader): add TODO comment for BindOncePromise.
pichlermarc 2de4fc9
fix(metric-reader): do not throw on collect and forceFlush when insta…
pichlermarc 056ea19
refactor(metric-reader): remove empty options from calls in MetricCol…
pichlermarc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
95 changes: 95 additions & 0 deletions
95
...ental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
* Copyright The 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 | ||
* | ||
* https://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 * as api from '@opentelemetry/api'; | ||
import { MetricReader } from './MetricReader'; | ||
import { MetricExporter } from './MetricExporter'; | ||
import { callWithTimeout, TimeoutError } from '../utils'; | ||
|
||
export type PeriodicExportingMetricReaderOptions = { | ||
exporter: MetricExporter | ||
exportIntervalMillis?: number, | ||
exportTimeoutMillis?: number | ||
} | ||
|
||
/** | ||
* {@link MetricReader} which collects metrics based on a user-configurable time interval, and passes the metrics to | ||
* the configured {@link MetricExporter} | ||
*/ | ||
export class PeriodicExportingMetricReader extends MetricReader { | ||
private _interval?: ReturnType<typeof setInterval>; | ||
|
||
private _exporter: MetricExporter; | ||
|
||
private readonly _exportInterval: number; | ||
|
||
private readonly _exportTimeout: number; | ||
|
||
constructor(options: PeriodicExportingMetricReaderOptions) { | ||
super(options.exporter.getPreferredAggregationTemporality()); | ||
|
||
if (options.exportIntervalMillis !== undefined && options.exportIntervalMillis <= 0) { | ||
throw Error('exportIntervalMillis must be greater than 0'); | ||
} | ||
|
||
if (options.exportTimeoutMillis !== undefined && options.exportTimeoutMillis <= 0) { | ||
throw Error('exportTimeoutMillis must be greater than 0'); | ||
} | ||
|
||
if (options.exportTimeoutMillis !== undefined && | ||
options.exportIntervalMillis !== undefined && | ||
options.exportIntervalMillis < options.exportTimeoutMillis) { | ||
throw Error('exportIntervalMillis must be greater than or equal to exportTimeoutMillis'); | ||
} | ||
|
||
this._exportInterval = options.exportIntervalMillis ?? 60000; | ||
this._exportTimeout = options.exportTimeoutMillis ?? 30000; | ||
this._exporter = options.exporter; | ||
} | ||
|
||
private async _runOnce(): Promise<void> { | ||
const metrics = await this.collect({}); | ||
await this._exporter.export(metrics); | ||
} | ||
|
||
protected override onInitialized(): void { | ||
// start running the interval as soon as this reader is initialized and keep handle for shutdown. | ||
this._interval = setInterval(async () => { | ||
try { | ||
await callWithTimeout(this._runOnce(), this._exportTimeout); | ||
} catch (err) { | ||
if (err instanceof TimeoutError) { | ||
api.diag.error('Export took longer than %s milliseconds and timed out.', this._exportTimeout); | ||
return; | ||
} | ||
|
||
api.diag.error('Unexpected error during export: %s', err); | ||
} | ||
}, this._exportInterval); | ||
} | ||
|
||
protected async onForceFlush(): Promise<void> { | ||
await this._exporter.forceFlush(); | ||
} | ||
|
||
protected async onShutdown(): Promise<void> { | ||
if (this._interval) { | ||
clearInterval(this._interval); | ||
} | ||
|
||
await this._exporter.shutdown(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, is this taking the Java route of passing a metric-producer rather than hard-coding
collect
to talk to static registry of metrics.I'm wondering if we should update the specification so that this is the "default" specified mechanism and the .NET hook is an alternative option to make it more clear to implementers that wish to a hard-link between metric-reader implementation + the sdk. Did you find this confusing at all?
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.
It is confusing on the spec side. I've previously submitted issues like open-telemetry/opentelemetry-specification#2072 for it.
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 agree that it is somewhat confusing on the spec side, when I was reading it the same question described in the issue above popped into my mind. I think updating the spec would help in that regard.