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

Fix bug with multiple views mapping to a single metric stream #3006

Merged
merged 32 commits into from
Apr 6, 2022

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Mar 10, 2022

My original implementation from #2916 did not correctly handle some cases when using views.

@alanwest alanwest marked this pull request as ready for review March 11, 2022 00:14
@alanwest alanwest requested a review from a team March 11, 2022 00:14
@alanwest alanwest marked this pull request as draft March 12, 2022 01:47
@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #3006 (7b70b30) into main (d294140) will increase coverage by 0.10%.
The diff coverage is 90.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3006      +/-   ##
==========================================
+ Coverage   84.66%   84.77%   +0.10%     
==========================================
  Files         259      260       +1     
  Lines        9228     9273      +45     
==========================================
+ Hits         7813     7861      +48     
+ Misses       1415     1412       -3     
Impacted Files Coverage Δ
...enTelemetry/Metrics/StringArrayEqualityComparer.cs 80.00% <80.00%> (ø)
src/OpenTelemetry/Metrics/MetricStreamIdentity.cs 90.00% <90.00%> (ø)
src/OpenTelemetry/Metrics/Metric.cs 95.65% <100.00%> (ø)
src/OpenTelemetry/Metrics/MetricReaderExt.cs 86.06% <100.00%> (+0.11%) ⬆️
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs 61.11% <0.00%> (+11.11%) ⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 50.00% <0.00%> (+14.28%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 59.09% <0.00%> (+22.72%) ⬆️

@alanwest alanwest marked this pull request as ready for review March 17, 2022 00:42
@alanwest alanwest changed the title Fix bug with multiple views selecting one instrument Fix bug with multiple views mapping to a single metric stream Mar 17, 2022
}
}

public readonly string MeterName { get; }
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 think we need readonly on all the members because the struct itself is readonly. Any harm in having it? Nothing major that I can think of. Might lead to a bigger binary, I'm assuming readonly ends up a hint in the IL metadata. More code to maintain 😄

@cijothomas cijothomas added this to the 1.2.0 milestone Mar 22, 2022

namespace OpenTelemetry.Metrics
{
internal class StringArrayEqualityComparer : IEqualityComparer<string[]>
Copy link
Member

Choose a reason for hiding this comment

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

@alanwest I just took a quick look, I think there might be some runtime bits we can use for these array comparers. Check out this fiddle:

using System;
using System.Collections;
using System.Collections.Generic;
					
public class Program
{
	public static void Main()
	{
		string[] first = new string[] { "one", "two", "three" };
		string[] second = new string[] { "one", "two", "three" };
		string[] third = new string[] { "one", "two", "THREE" };
		
		Console.WriteLine(((IStructuralEquatable)first).Equals(second, StringComparer.Ordinal));
		Console.WriteLine(((IStructuralEquatable)first).Equals(third, StringComparer.Ordinal));
		Console.WriteLine(((IStructuralEquatable)first).Equals(third, StringComparer.OrdinalIgnoreCase));
		
		double[] firstD = new double[] { 0.18D, 1D, 0.99D };
		double[] secondD = new double[] { 0.18D, 1D, 0.99D };
		
		Console.WriteLine(((IStructuralEquatable)firstD).Equals(secondD, EqualityComparer<double>.Default));
	}
}

It looks like Array has this hidden (explicitly implemented) IStructuralEquatable we might be able to use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool I like this. From looking at reference source looks like Array has implemented IStructuralEquatable for quite some time.

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 reverted my change to use IStructuralEquatable. Unit tests were failing only for net461. I haven't dug into why. I'd like to follow up with trying out IStructureEquatable in a separate PR because it definitely does simplify the code.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 30, 2022
@alanwest alanwest removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 30, 2022
@alanwest alanwest force-pushed the alanwest/view-bug branch from 3bfed37 to 931ea28 Compare March 31, 2022 06:04
@alanwest
Copy link
Member Author

alanwest commented Apr 6, 2022

Merging this. There are a few follow ups coming soon.

  • There's still a case this PR is not handling correctly. Namely multiple view configs that generate identical metric streams with this PR are "merged" into one stream. Each view should in fact result in a separate stream.
  • Tests from this PR should be moved to MetricViewTests.cs.
  • IStructuralEquatable may help simplify some of this code.

@alanwest alanwest merged commit 2360973 into open-telemetry:main Apr 6, 2022
@alanwest alanwest deleted the alanwest/view-bug branch April 6, 2022 22:30
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