Skip to content

WIP: O11Y-2154: Implement MeterProvider #73

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

Closed
wants to merge 12 commits into from
Closed

Conversation

dustinlessard-wf
Copy link
Contributor

No description provided.

@aviary3-wk
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@semveraudit-wf
Copy link

semveraudit-wf commented Aug 24, 2022

Public API Changes

Recommendation: ⚠️ Minor version bump

@@ line 4: package:opentelemetry/src/sdk/common/constants.dart @@
+  String get invalidMeterNameMessage
// Adding a top-level variable is a minor change.
@@ line 7: package:opentelemetry/src/api/metrics/noop/noop_meter.dart @@
+  class NoopMeter implements Meter
// Adding a class is a minor change.

@@ line 6: package:opentelemetry/src/sdk/metrics/metric_options.dart @@
+  class MetricOptions implements MetricOptions
// Adding a class is a minor change.

@@ line 7: package:opentelemetry/src/api/metrics/noop/noop_counter.dart @@
+  class NoopCounter<t> implements Counter<dynamic>
// Adding a class is a minor change.

@@ line 5: package:opentelemetry/src/api/metrics/metric_options.dart @@
+  abstract class MetricOptions
// Adding a class is a minor change.

Click to see 7 more API Changes


@@ line 7: package:opentelemetry/src/api/metrics/noop/noop_meter_provider.dart @@
+  class NoopMeterProvider implements MeterProvider
// Adding a class is a minor change.

@@ line 8: package:opentelemetry/src/sdk/metrics/meter_provider.dart @@
+  class MeterProvider implements MeterProvider
// Adding a class is a minor change.

@@ line 7: package:opentelemetry/src/sdk/metrics/meter.dart @@
+  class Meter implements Meter
// Adding a class is a minor change.

@@ line 6: package:opentelemetry/src/api/metrics/meter.dart @@
+  abstract class Meter
// Adding a class is a minor change.

@@ line 6: package:opentelemetry/src/api/metrics/counter.dart @@
+  abstract class Counter<t>
// Adding a class is a minor change.

@@ line 6: package:opentelemetry/src/sdk/metrics/counter.dart @@
+  class Counter<t> implements Counter<dynamic>
// Adding a class is a minor change.

@@ line 7: package:opentelemetry/src/api/metrics/meter_provider.dart @@
+  abstract class MeterProvider
// Adding a class is a minor change.


Showing results for 12630a0

Powered by semver-audit-service. Please report any problems by filing an issue.
Reported by the dart semver audit client 2.2.2
Browse public API.

Last edited UTC Sep 04 at 14:17:51

@dustinlessard-wf dustinlessard-wf changed the title WIP: O11Y-2154: added abstract meter related classes WIP: O11Y-2154: Implement MeterProvider Aug 28, 2022
@dustinlessard-wf
Copy link
Contributor Author

@evanweible-wf @blakeroberts-wk , I'd like your input on a pattern I am questioning.

The OTEL specification organizes code into two main packages: API and SDK. As you may know,
The API includes cross-cutting public interfaces used for instrumentation as well as default no-op implementations. Libraries are supposed to only depend on the API.
The SDK implements the API and is installed and managed by the application owner.

Many languages implement the OTEL spec using 3 main constructs:

  • Abstract classes (In the API)
  • Noop classes (In the API)
  • Concrete SDK classes

I propose that since Dart classes can be treated as interfaces, we forego the abstract classes so that we only have two main constructs:

  • Classes that are in the API include the default noop implementation built in
  • Classes that are in the SDK implement the API classes

Pros

  • simplifies the codebase

Cons

  • deviates from typical code organization pattern

What do y'all think about this? Bad idea? Meh? Shrug? Awesome?

@blakeroberts-wk
Copy link
Contributor

I'd add one more con: it may be less obvious that a class is noop. Like the API implementation might be NoopCounter, making it obvious what that thing does. But even so, the complexity of two classes probably isn't as readable as using the same class. I'm on board.

@evanweible-wf
Copy link
Contributor

evanweible-wf commented Aug 29, 2022

I propose that since Dart classes can be treated as interfaces

FWIW, while this is true, the language team is actively discussing proposals to introduce more control over types, which would include the ability to define an actual interface.

Personally I would vote for having separate interfaces and no-op default implementations. Doing so gives you clearer separation of concerns, where each can be written and documented for its target audiences. The interface for implementers as well as consumers who want to know what the type means and what it should do, without needing to care about implementation-specific details; the no-ops can defer mostly to the interface, while making it clear to consumers that they indeed do nothing; and the concrete SDK implementations can explain how they implement the interface and when/how they should be used.

Combining the interfaces with the default no-op implementations also means that people could extend those classes without needing to implement every part of the interface (since they'd inherit the no-op behaviors by default). This creates a potential pitfall where they might implement only a subset of an interface, giving them an incomplete implementation.

@dustinlessard-wf
Copy link
Contributor Author

Thanks gents! I'm going to rework this to separate the Noop implementations from the API classes. I'll switch to using abstract classes that will force implementers to implement the required functions.

@danrick-wk danrick-wk mentioned this pull request Sep 9, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants