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.

Sign up for GitHub

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

Jump to bottom

Apply design feedback on Metrics and DistributedContext #340

Merged
merged 5 commits into from
Nov 20, 2019
Merged

Apply design feedback on Metrics and DistributedContext #340

merged 5 commits into from
Nov 20, 2019

Conversation

tarekgh
Copy link
Contributor

@tarekgh tarekgh commented Nov 13, 2019 •
edited
Loading

  • Reduce the number of abstract methods in Meter and allow overriding protected methods to avoid make it public.
  • Rename the word “Long” in the APIs to “Int64”. This is the design guidelines recommendation.
  • Add convenient properties NoPropagationEntry and UnlimitedPropagationEntry in EntryMetadata.
  • Avoid using the 3 uppercase letters acronym of TTL and just use the full words TimeToLive.

@tarekgh
Apply design feedback on Metrics and DistributedContext
807727f 
- Reduce the number od abstract methods on Meter and allow overriding protected methods to avoid make it public.
- Rename the word “Long” in the APIs to “Int64”. This is the design guidelines recommendation.
- Add convenient properties NoPropagationEntry and UnlimitedPropagationEntry in EntryMetadata.
- Avoid using the 3 uppercase letters acronym of TTL and just use the full words TimeToLive.
@tarekgh
Copy link
Contributor Author

tarekgh commented Nov 13, 2019

@lmolkova @SergeyKanzhelev @cijothomas could you please have a quick look? Thanks.

MikeGoldsmith
MikeGoldsmith suggested changes Nov 19, 2019
View reviewed changes
Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

I like the changes to EntryMetada and adding generic overloads to Meter. Just a couple of smaller suggestions.

src/OpenTelemetry.Api/DistributedContext/EntryMetadata.cs Outdated Show resolved Hide resolved
src/OpenTelemetry.Api/DistributedContext/EntryMetadata.cs Outdated Show resolved Hide resolved
src/OpenTelemetry.Api/Metrics/Meter.cs Outdated Show resolved Hide resolved
src/OpenTelemetry.Api/Metrics/Meter.cs Outdated Show resolved Hide resolved
@tarekgh
Copy link
Contributor Author

tarekgh commented Nov 19, 2019

I think the failures here are not related to my changes. Could someone take a look?

[xUnit.net 00:00:03.72]     OpenTelemetry.Trace.Export.Test.BatchingSpanProcessorTest.ProcessorDoesNotBlockOnExporter [FAIL]
  X OpenTelemetry.Trace.Export.Test.BatchingSpanProcessorTest.ProcessorDoesNotBlockOnExporter [1s 44ms]
  Error Message:
   Expected at least 1, got 0
Expected: True
Actual:   False
  Stack Trace:
     at OpenTelemetry.Trace.Export.Test.BatchingSpanProcessorTest.WaitForSpans(TestExporter exporter, Int32 spanCount, TimeSpan timeout) in d:\a\1\s\test\OpenTelemetry.Tests\Impl\Trace\Export\BatchingSpanProcessorTests.cs:line 341
   at OpenTelemetry.Trace.Export.Test.BatchingSpanProcessorTest.ProcessorDoesNotBlockOnExporter() in d:\a\1\s\test\OpenTelemetry.Tests\Impl\Trace\Export\BatchingSpanProcessorTests.cs:line 243

@lmolkova
Copy link

lmolkova commented Nov 19, 2019 •
edited
Loading

I think the failures here are not related to my changes. Could someone take a look?

Agree. Test is flaky. Could you create an issue - I'll fix it later?

@tarekgh
Copy link
Contributor Author

tarekgh commented Nov 19, 2019

created #344

@lmolkova lmolkova merged commit d44cea3 into open-telemetry:master Nov 20, 2019
@tarekgh tarekgh deleted the DesignFeedback1 branch August 31, 2022 15:46
Yun-Ting pushed a commit to Yun-Ting/opentelemetry-dotnet that referenced this pull request Oct 13, 2022
@reyang
Editorial changes to the README.md (open-telemetry#340)
fe42963
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeGoldsmith MikeGoldsmith MikeGoldsmith approved these changes

@lmolkova lmolkova lmolkova approved these changes

@austinlparker austinlparker Awaiting requested review from austinlparker

@bruno-garcia bruno-garcia Awaiting requested review from bruno-garcia

@discostu105 discostu105 Awaiting requested review from discostu105

@SergeyKanzhelev SergeyKanzhelev Awaiting requested review from SergeyKanzhelev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tarekgh @lmolkova @MikeGoldsmith @SergeyKanzhelev