Skip to content
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

Metric Push Controller and basic config #599

Merged
merged 25 commits into from
Apr 16, 2020
Merged

Metric Push Controller and basic config #599

merged 25 commits into from
Apr 16, 2020

Conversation

cijothomas
Copy link
Member

Added PushController for Metrics.
Users can configure the Push interval, Batcher, Exporter using MeterBuilder, MeterFactory

(There are no SDK specs yet. Following similar approach as Tracer. These may require changes once Meter SDK specs are finalized)

#409 #390

}

[Event(18, Message = "Collection completed in '{0}' msecs.", Level = EventLevel.Informational)]
public void CollectionCompleted(long msec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: seems like an unnecessary conversion of StopWatch to milliseconds. Would ticks work here?

public abstract void Process(string meterName, string metricName, LabelSet labelSet, Aggregator<double> aggregator);

/// <summary>
/// Finish the current collection cycle and return the metrics.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this method does based on this description. Can you please make it more elaborate. Especially as it's very different from the SpanProcessor

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments to make it clear that this is called at the end of every collection cycles. MetricProcessor can chose to clear it state (if its stateless) at this call.
(The spec for this are not yet final, so there may be some changes later)

This is not same as ShutDownAsync for SpanProcessor

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments.. Biggest feedback I don't understand what's the new method on processor does

@cijothomas cijothomas closed this Apr 15, 2020
@cijothomas cijothomas reopened this Apr 15, 2020
@cijothomas cijothomas closed this Apr 15, 2020
@cijothomas cijothomas reopened this Apr 15, 2020
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the minor recommendations.

@cijothomas
Copy link
Member Author

@SergeyKanzhelev Thanks for detailed review! Addressed all of them. Please check again.

@cijothomas
Copy link
Member Author

@SergeyKanzhelev Please merge. I have follow up PRs waiting for this to be merged.

Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks @cijothomas 👍

@SergeyKanzhelev SergeyKanzhelev merged commit 2e7e93c into open-telemetry:master Apr 16, 2020
Yun-Ting pushed a commit to Yun-Ting/opentelemetry-dotnet that referenced this pull request Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants