Add view (configuration only) by cijothomas · Pull Request #2396 · open-telemetry/opentelemetry-dotnet

cijothomas · 2021-09-21T15:58:23Z

Adds ability to add Views.
Documents/example.

In next PRs: Actual implementation.

reyang · 2021-09-22T22:51:53Z

+    {
+        using var meterProvider = Sdk.CreateMeterProviderBuilder()
+            .AddSource(MyMeter.Name)
+            .AddView(instrumentName: "MyCounter") // MyCounter will be reported with defaults. This is done to ensure that any other wildcard Views for the same isntrument does not affect this instrument.


@mic-max @Yun-Ting wish to get your feedback on these regarding the usability / easy-to-understand part.

my gut says, unless one understands View's concept from the spec (and concept of metric stream), this is not super intuitive. :)

I see you have a class for MetricStreamConfig. How about a class for SelectionCriteria as well?

So the signature would be AddView(SelectionCriteria, MetricStreamConfig).

Later we might consider some builder methods for these classes as well. Something like:

var selectionCriteria = SelectionCriteria .Create() .MeterName("SomeMeter") .InstrumentName("SomeInstrument"); var viewConfig = MetricStreamConfig .Create() .Name("NewNameForSomeMeter") .Aggregation(Aggregation.Histogram);

MetricStreamConfig is internal (my bad missed removing it from public!).
User doesn't see that.

mic-max · 2021-09-23T15:41:52Z

+                    selectInstrument &= true;
+                }
+
+                if (!string.IsNullOrWhiteSpace(meterVersion)


if selectInstrument is already true these checks can be skipped

Since these 3 string are already checked for IsNullOrWhiteSpace should we cache that value? Or can we indicate to the compiler this string will never change, make them in arguments? https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/in-parameter-modifier

mic-max · 2021-09-23T15:51:37Z

+                        incomingInstrumentType = InstrumentType.Histogram;
+                    }
+
+                    if (incomingInstrumentType == instrumentType)


Could this be the body of an else statement

mic-max

I think the end user experience is good. I think I understood at least how to set things up. The wildcard part and having to put certain views above that one could be an issue I see people running into if they're not very conscious of that, especially when those strings might not be literals and be loaded from a less visible config or environment variable.

codecov · 2021-09-23T20:09:19Z

Codecov Report

Merging #2396 (bebed03) into main (400e463) will decrease coverage by 0.23%.
The diff coverage is 22.58%.

@@            Coverage Diff             @@
##             main    #2396      +/-   ##
==========================================
- Coverage   80.19%   79.95%   -0.24%     
==========================================
  Files         235      237       +2     
  Lines        7569     7599      +30     
==========================================
+ Hits         6070     6076       +6     
- Misses       1499     1523      +24

Impacted Files	Coverage Δ
...rc/OpenTelemetry/Metrics/HistogramConfiguration.cs	`0.00% <0.00%> (ø)`
...elemetry/Metrics/MeterProviderBuilderExtensions.cs	`40.00% <0.00%> (-32.73%)`	⬇️
...OpenTelemetry/Metrics/MetricStreamConfiguration.cs	`0.00% <0.00%> (ø)`
.../OpenTelemetry/Metrics/MeterProviderBuilderBase.cs	`82.92% <40.00%> (-5.97%)`	⬇️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs	`91.58% <100.00%> (+0.32%)`	⬆️

cijothomas · 2021-09-23T20:12:51Z

From offline discussions with @alanwest

It might be easier to provide strong classes like InstrumentSelectionCriteria (metername,version,instrumentname,type), MetricConfig(name,agg,custombounds.)
The above might make simple cases require more code.
eg: Simple rename:

           AddView(name: "httpRequestsOutbound", instrumentName: "httpRequests")
vs
           AddView(new InstrumentSelectionCriteria(){instrumentName="httpRequests"}, new MetricConfig(){name="httpRequestsOutbound"})

A common use case for Views is histogram bounds. It can be achieved like this now

.AddView(instrumentType: InstrumentType.Histogram, histogramBounds: new double[] { 100, 200 })

We may consider offering extension methods like SetDefaultHistogramBounds(100,200), which will not force user to ever know about views at all as all they want is a simple change to histogram bounds. (as long as its for the entire histograms!)

reyang · 2021-09-23T20:21:53Z

+    {
+        using var meterProvider = Sdk.CreateMeterProviderBuilder()
+            .AddSource(MyMeter.Name)
+            .AddView(instrumentName: "MyCounter") // MyCounter will be reported with defaults. This is done to ensure that any other wildcard Views for the same instrument does not affect this instrument.


minor, these lines are too long, consider making them block comments, e.g.:

using var meterProvider = Sdk.CreateMeterProviderBuilder() .AddSource(MyMeter.Name) /* MyCounter will be reported with defaults. This is done to ensure that any other wildcard Views for the same instrument does not affect this instrument. */ .AddView(instrumentName: "MyCounter")

Agree.
Also I plan to move most of these comments to the readme, to make the program cleaner.
A readme section showing View examples should be very useful.

reyang · 2021-09-24T01:43:59Z

+using System.Collections.Generic;
+using System.Text;
+
+namespace OpenTelemetry.Metrics


Let's have offline discussion, I think a hierarchical design might help us to clean this up.

reyang

https://github.com/open-telemetry/opentelemetry-dotnet/pull/2396/files#r715262564

cijothomas · 2021-09-27T21:59:26Z

+            .AddView((instrument) =>
+             {
+                 if (instrument.Meter.Name.Equals("CompanyA.ProductB.Library2") &&
+                     instrument.GetType().Name.StartsWith("Histogram"))


might benefit from some extension method like instrument.IsHistogram?

I like this idea - fine as a follow up probably.

Maybe something like instrument.InstrumentType that returns an enum'ed value.

reyang · 2021-09-27T22:06:16Z

+            .AddView(instrumentName: "MyCounterCustomTags", new AggregationConfig() { TagKeys = new string[] { "tag1", "tag2" } })
+
+            // Drop the instrument "MyCounterDrop".
+            .AddView(instrumentName: "MyCounterDrop", new DropAggregationConfig())


This is a bit weird.
Consider something like new AggregationConfig { Type = None }?

Looks like this got commented out for now, I do like new AggregationConfig { Type = None }. Another idea is having some static AggregationConfigs for common scenarios like

class AggregationConfig { static AggregationConfig None = new AggregationConfig() { Aggregation = Aggregation.None }; }

Hmm, maybe nevermind because then people could mutate the static configs.

yes, this is something which can potentially be improved.

reyang · 2021-09-27T22:06:39Z

+            .AddView(instrumentName: "MyCounter", name: "MyCounterRenamed")
+
+            // Change Histogram bounds
+            .AddView(instrumentName: "MyHistogram", new HistogramConfig() { HistogramBounds = new double[] { 10, 20 } })


Suggested change

.AddView(instrumentName: "MyHistogram", new HistogramConfig() { HistogramBounds = new double[] { 10, 20 } })

.AddView(instrumentName: "MyHistogram", new HistogramConfiguration { BucketBounds = new { 10, 20 } })

I don't know which one is better HistogramBounds, BucketBounds or just Bounds. HistogramBounds seems to be bad because the type already suggested that it is Histogram.

Looks much better now.

alanwest

Put a few thoughts, but this is looking 👍

reyang

LGTM.

## Changes Update the compliance matrix for .NET as of 1.16.0 of the .NET SDK. I got Copilot to do most of the work of researching what was missing that had been implementing and finding the relevant PRs that implemented then. I cross checked the PRs _looked_ correct, but some of it might be incorrect, particularly things that seem to have been implemented for a while and predate my contributions to OTel. I then had Claude review the changes and cross-reference, which fixed some of the entries, but again there might still be some mistakes. ~~Some of the entries might only be available as experimental features, so maybe they shouldn't be updated?~~ Experimental entries removed, now marked as ~~strikethrough~~ below. Entries updated with relevant PR links where appropriate below. /cc @open-telemetry/dotnet-maintainers ### Traces - Get a Tracer with schema_url: open-telemetry/opentelemetry-dotnet#6736 - Get a Tracer with scope attributes: open-telemetry/opentelemetry-dotnet#6137 - Associate Tracer with InstrumentationScope: open-telemetry/opentelemetry-dotnet#239 - Fetch InstrumentationScope from ReadableSpan: open-telemetry/opentelemetry-dotnet#679 ### Metrics - Associate Meter with InstrumentationScope: open-telemetry/opentelemetry-dotnet#6714 - Gauge instrument is supported: open-telemetry/opentelemetry-dotnet#5867 - Valid instrument created / warning for duplicate name: open-telemetry/opentelemetry-dotnet#2916 - Duplicate instrument registration resolved by first-seen: open-telemetry/opentelemetry-dotnet#2916 - Instrument names conform to specified syntax: open-telemetry/opentelemetry-dotnet#3821 - Instrument supports advisory ExplicitBucketBoundaries: open-telemetry/opentelemetry-dotnet#5854 - Instrument supports advisory Attributes: Not implemented - name/version/schema_url create InstrumentationScope: open-telemetry/opentelemetry-dotnet#6714 - ~~View allows configuring name, description, attributes, aggregation: open-telemetry/opentelemetry-dotnet#2396 - ~~View allows configuring exemplar reservoir: open-telemetry/opentelemetry-dotnet#5558 - Metrics Reader supports default aggregation config: open-telemetry/opentelemetry-dotnet#6778 - Metrics Reader supports default temporality config: open-telemetry/opentelemetry-dotnet#4667 - ~~ExemplarReservoir interface/extension point: open-telemetry/opentelemetry-dotnet#5542 - ~~ExemplarReservoir has offer method: open-telemetry/opentelemetry-dotnet#5542 - SimpleFixedSizeExemplarReservoir: open-telemetry/opentelemetry-dotnet#4256 - AlignedHistogramBucketExemplarReservoir: open-telemetry/opentelemetry-dotnet#4119 ### Logs - ~~LoggerProvider.GetLogger: open-telemetry/opentelemetry-dotnet#4422 - LoggerProvider.GetLogger accepts attributes: Not implemented - LoggerProvider.Shutdown: open-telemetry/opentelemetry-dotnet#5648 - LoggerProvider.ForceFlush: open-telemetry/opentelemetry-dotnet#5648 - Logger.Emit(LogRecord) with Exception parameter: Not implemented - ~~LogRecord.Set EventName: open-telemetry/opentelemetry-dotnet#6306 - Logger.Enabled: Not implemented - Ergonomic API (ILogger bridge): open-telemetry/opentelemetry-dotnet#3489 - SimpleLogRecordProcessor: open-telemetry/opentelemetry-dotnet#1622 - BatchLogRecordProcessor: open-telemetry/opentelemetry-dotnet#1622 - Can plug custom LogRecordProcessor: open-telemetry/opentelemetry-dotnet#4916 - LogRecordProcessor.Enabled: Not implemented - Can plug custom LogRecordExporter: open-telemetry/opentelemetry-dotnet#1622 ### Context Propagation - Jaeger Propagator: open-telemetry/opentelemetry-dotnet#3309 - TextMapPropagator: open-telemetry/opentelemetry-dotnet#1427 ### Environment Variables - OTEL_SDK_DISABLED: open-telemetry/opentelemetry-dotnet#6568 - OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT: open-telemetry/opentelemetry-dotnet#4887 - OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT: open-telemetry/opentelemetry-dotnet#4887 - OTEL_TRACES_SAMPLER: open-telemetry/opentelemetry-dotnet#5448 - OTEL_TRACES_SAMPLER_ARG: open-telemetry/opentelemetry-dotnet#5448 - OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION: open-telemetry/opentelemetry-dotnet#6778 ### Exporters — OTLP - OTLP/HTTP gzip Content-Encoding support: open-telemetry/opentelemetry-dotnet#7055 - ~~Honors retryable responses with backoff: open-telemetry/opentelemetry-dotnet#5495 - ~~Honors non-retryable responses: open-telemetry/opentelemetry-dotnet#5495 - ~~Honors throttling response: open-telemetry/opentelemetry-dotnet#5495 - Metric Exporter configurable temporality preference: open-telemetry/opentelemetry-dotnet#4667 - Metric Exporter configurable default aggregation: open-telemetry/opentelemetry-dotnet#6778 ### Exporters — Zipkin - InstrumentationScope mapping: open-telemetry/opentelemetry-dotnet#5473 ### Exporters — Prometheus - Metadata Deduplication: open-telemetry/opentelemetry-dotnet#7237 - otel_scope_name and otel_scope_version labels: open-telemetry/opentelemetry-dotnet#7237 - otel_scope_[attribute] labels: open-telemetry/opentelemetry-dotnet#7237 - Prometheus Counters have _total suffix by default: open-telemetry/opentelemetry-dotnet#5305 - Prometheus Counters _total suffixing can be disabled: open-telemetry/opentelemetry-dotnet#5305 - Colliding sanitized attribute keys are merged: open-telemetry/opentelemetry-dotnet#7239 - Exemplars for Histograms and Monotonic sums: open-telemetry/opentelemetry-dotnet#7222 - target_info metric from Resource: open-telemetry/opentelemetry-dotnet#5407 ### OpenTracing Compatibility - Create OpenTracing Shim: open-telemetry/opentelemetry-dotnet#197 - Tracer shim: open-telemetry/opentelemetry-dotnet#197 - Span shim: open-telemetry/opentelemetry-dotnet#197 - SpanContext shim: open-telemetry/opentelemetry-dotnet#197 - ScopeManager shim: open-telemetry/opentelemetry-dotnet#197 - Error mapping for attributes/events: open-telemetry/opentelemetry-dotnet#197

Discuss view config option

059d1a0

cijothomas commented Sep 21, 2021

View reviewed changes

Comment thread docs/metrics/customizing-the-sdk/Program.cs Outdated

cijothomas commented Sep 21, 2021

View reviewed changes

Comment thread docs/metrics/customizing-the-sdk/Program.cs Outdated

cijothomas commented Sep 21, 2021

View reviewed changes

Comment thread docs/metrics/customizing-the-sdk/Program.cs Outdated

cijothomas added 3 commits September 22, 2021 14:21

simpler api

c0a377d

adjust names

3a16123

add drop instrument

f40f613

cijothomas marked this pull request as ready for review September 22, 2021 21:54

cijothomas requested a review from a team September 22, 2021 21:54

reyang reviewed Sep 22, 2021

View reviewed changes

cijothomas added 2 commits September 22, 2021 16:44

mark config internal

e4ad84c

more selection criterian

47812de

mic-max reviewed Sep 23, 2021

View reviewed changes

Comment thread docs/metrics/customizing-the-sdk/Program.cs Outdated

mic-max reviewed Sep 23, 2021

View reviewed changes

Comment thread src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs Outdated

mic-max reviewed Sep 23, 2021

View reviewed changes

cijothomas added 4 commits September 23, 2021 10:22

adjuts words

81e2c4a

Merge branch 'main' into cijothomas/view_example1

55669f0

add readme

7cc17dc

fix logic

5c28f2e

cijothomas changed the title ~~Discuss view config option~~ Add view (configuration only) Sep 23, 2021

cijothomas added 4 commits September 23, 2021 12:56

Merge branch 'main' into cijothomas/view_example1

bfa877d

fix lint

6b40f06

rename

485c04e

typos

f939b53

reyang reviewed Sep 23, 2021

View reviewed changes

Comment thread docs/metrics/customizing-the-sdk/Program.cs Outdated

cijothomas mentioned this pull request Sep 23, 2021

basic View allowing to pick dimensions. One view per instrument #2210

Closed

cijothomas added 3 commits September 23, 2021 15:59

add drop to enum

4515a84

Merge branch 'main' into cijothomas/view_example1

70750fd

simple

df5939b

reyang reviewed Sep 24, 2021

View reviewed changes

reyang previously requested changes Sep 24, 2021

View reviewed changes

cijothomas added 8 commits September 27, 2021 10:08

Merge branch 'main' into cijothomas/view_example1

2c27fa6

conflict

7e4edd4

refactor example

2cc1081

fix build

d71eb1e

comment fix

5db05b3

cleanup

ef9a842

default

0764a7c

less public

b9f287c

cijothomas commented Sep 27, 2021

View reviewed changes

reyang reviewed Sep 27, 2021

View reviewed changes

cijothomas added 2 commits September 27, 2021 15:54

few twaks

31fa994

Merge branch 'main' into cijothomas/view_example1

48290f0

alanwest reviewed Sep 27, 2021

View reviewed changes

Comment thread src/OpenTelemetry/Metrics/HistogramConfiguration.cs Outdated

alanwest approved these changes Sep 27, 2021

View reviewed changes

reyang approved these changes Sep 28, 2021

View reviewed changes

cijothomas added 2 commits September 27, 2021 20:31

rename aggregationconfig to MetricStreamConfiguration

e908642

mark todos

bebed03

cijothomas merged commit 2c70ca6 into open-telemetry:main Sep 28, 2021

cijothomas deleted the cijothomas/view_example1 branch September 28, 2021 03:47

alanwest mentioned this pull request Oct 5, 2021

View - support for dropping instrument #2449

Merged

martincostello mentioned this pull request Jun 25, 2026

Update .NET compliance matrix entries open-telemetry/opentelemetry-specification#5176

Merged

	.AddView(instrumentName: "MyHistogram", new HistogramConfig() { HistogramBounds = new double[] { 10, 20 } })
	.AddView(instrumentName: "MyHistogram", new HistogramConfiguration { BucketBounds = new { 10, 20 } })

Uh oh!

Conversation

cijothomas commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanwest Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mic-max left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cijothomas commented Sep 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanwest Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alanwest left a comment

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cijothomas commented Sep 21, 2021 •

edited

Loading

alanwest Sep 22, 2021 •

edited

Loading

codecov Bot commented Sep 23, 2021 •

edited

Loading

alanwest Sep 27, 2021 •

edited

Loading

reyang Sep 27, 2021 •

edited

Loading