Skip to content

Conversation

@utpilla
Copy link
Contributor

@utpilla utpilla commented Nov 30, 2021

Reverts #2705

We want to avoid the situation where a user can add a statement such as:

.AddView(instrumentName: "MyCounter", new HistogramConfiguration() { Name = "newname"})
This will not update the Counter to a Histogram

Since this will not be a breaking change if added after 1.2.0 release, we can add it later.

Documentation on types of breaking changes: https://docs.microsoft.com/en-us/dotnet/core/compatibility/#types

❓ REQUIRES JUDGMENT: Introducing a new base class

A type can be introduced into a hierarchy between two existing types if it doesn't introduce any new abstract members or change the semantics or behavior of existing types. For example, in .NET Framework 2.0, the DbConnection class became a
new base class for SqlConnection, which had previously derived directly from Component.

I also manually ran the APICompat tool to ensure that this will not be a breaking change if we decide to add it later on.

@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #2706 (32e96fc) into main (f5b0ea3) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2706      +/-   ##
==========================================
- Coverage   84.07%   84.07%   -0.01%     
==========================================
  Files         251      250       -1     
  Lines        8778     8777       -1     
==========================================
- Hits         7380     7379       -1     
  Misses       1398     1398              
Impacted Files Coverage Δ
...ry/Metrics/ExplicitBucketHistogramConfiguration.cs 100.00% <ø> (ø)

@cijothomas
Copy link
Member

@utpilla showed that we still have this issue today with ExplicitHistogramConfiguration. So may be okay to leave this in.
And add a validation in AddView wherever feasible to fail-fast.

@cijothomas cijothomas merged commit a828aa7 into main Nov 30, 2021
@cijothomas cijothomas deleted the revert-2705-cijothomas/histogramminmax branch November 30, 2021 01:28
@cijothomas
Copy link
Member

@utpilla showed that we still have this issue today with ExplicitHistogramConfiguration. So may be okay to leave this in. And add a validation in AddView wherever feasible to fail-fast.

Merging this PR (i.e reverting the newly added confusion)

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.

5 participants