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

Prototype of a very basic Aggregation-configuration API in the SDK. #1412

Closed
wants to merge 13 commits into from

Conversation

jkwatson
Copy link
Contributor

@jkwatson jkwatson commented Jul 14, 2020

Note: related OTEP: open-telemetry/oteps#126

This prototype supports the following:

  • select instruments to customize by instrument type and name.
  • specify the aggregation and the temporality of the aggregation to be used.

An example of how you could use it:

// get a handle to the MeterSdkProvider
 MeterSdkProvider meterProvider = OpenTelemetrySdk.getMeterProvider();

 // create a selector to select which instruments to customize:
 InstrumentSelector instrumentSelector = InstrumentSelector.newBuilder()
  .instrumentType(InstrumentType.COUNTER)
  .build();

 // create a specification of how you want the metrics aggregated:
 ViewSpecification viewSpecification = 
      ViewSpecification.create(Aggregations.minMaxSumCount(), Temporality.DELTA);

 //register the view with the MeterSdkProvider
 meterProvider.registerView(instrumentSelector, viewSpecification);

@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #1412 into master will decrease coverage by 0.03%.
The diff coverage is 86.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1412      +/-   ##
============================================
- Coverage     92.50%   92.46%   -0.04%     
- Complexity      939      955      +16     
============================================
  Files           117      119       +2     
  Lines          3348     3385      +37     
  Branches        270      274       +4     
============================================
+ Hits           3097     3130      +33     
- Misses          169      173       +4     
  Partials         82       82              
Impacted Files Coverage Δ Complexity Δ
...va/io/opentelemetry/sdk/metrics/ActiveBatcher.java 87.50% <0.00%> (-12.50%) 4.00 <0.00> (ø)
...in/java/io/opentelemetry/sdk/metrics/Batchers.java 92.98% <50.00%> (-1.57%) 4.00 <0.00> (ø)
...io/opentelemetry/sdk/metrics/MeterSdkProvider.java 94.44% <50.00%> (-5.56%) 5.00 <1.00> (ø)
...ava/io/opentelemetry/sdk/metrics/ViewRegistry.java 88.23% <93.54%> (+24.59%) 16.00 <12.00> (+10.00)
...try/sdk/metrics/view/AggregationConfiguration.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...telemetry/sdk/metrics/view/InstrumentSelector.java 100.00% <100.00%> (ø) 4.00 <4.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ed649...cbbe742. Read the comment docs.

@jkwatson jkwatson changed the title Prototype of a very basic Views API in the SDK. Prototype of a very basic Aggregation-configuration API in the SDK. Jul 15, 2020
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

API seems reasonable to me, guess some more javadoc to add?

descriptor, meterProviderSharedState, meterSharedState, aggregation);
Aggregation aggregation = specification.aggregation();

if (Temporality.CUMULATIVE == specification.temporality()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we'd usually order specification.temporality() == or use a switch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen it both ways...I don't think there's much of a standard, or even agreement on this. Back in the day, I'd use .equals() even for enums, since someone could change it from an enum and break code if you use ==. :)

public abstract Temporality temporality();

/** An enumeration which describes the time period over which metrics should be aggregated. */
public enum Temporality {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure but it seems like this could be applicable as a top level enum (outside the realm of views)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possibly, yes. I think this will become a top-level concept eventually, so if/when that happens, we'll definitely make it more first-class.


private MeterSdkProvider(Clock clock, Resource resource) {
this.registry =
new MeterSdkComponentRegistry(
MeterProviderSharedState.create(clock, resource), new ViewRegistry());
MeterProviderSharedState.create(clock, resource), viewRegistry);
Copy link
Member

Choose a reason for hiding this comment

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

Wondering why we don't want ViewRegistry per MeterSdk instance level? What if I have Metric instruments with the same type and name but I want it have different custom aggregation & temporality on different instrumentationName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SDK user won't in general know what named Meters are available, so I don't know how you would access the specific Meter in order to have a registry at that level. How were you thinking that an operator would know how to use this if the API were available only on the individual Meters?

@tylerbenson
Copy link
Member

@jkwatson what needs to happen to make progress here? I feel this is a blocker for my work in adding an additional aggregation option.

@jkwatson
Copy link
Contributor Author

jkwatson commented Nov 6, 2020

@jkwatson what needs to happen to make progress here? I feel this is a blocker for my work in adding an additional aggregation option.

Well, aside from a rebase... we'd have to decide this was something we wanted to implement, even though it's not really spec'd at this point.

@jkwatson
Copy link
Contributor Author

jkwatson commented Nov 6, 2020

closing in favor of #2037 (rebasing this proved messy and I no longer work for NR, so didn't want to use their fork as the basis for the PR)

@jkwatson jkwatson closed this Nov 6, 2020
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.

4 participants