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

feat: Include endpoint URI in connection failure events #2686

Merged
merged 8 commits into from
Nov 29, 2021

Conversation

tomkerkhove
Copy link
Contributor

@tomkerkhove tomkerkhove commented Nov 25, 2021

Changes

Include endpoint URI in connection failure events to help improve troubleshooting to ensure that the called endpoint is configured correctly.

For significant contributions please make sure you have completed the following items:

  • Update changelog
  • [ ] Design discussion issue #
  • [ ] Changes in public API reviewed

Did not update public API given the event source is internal.

Signed-off-by: GitHub <[email protected]>
Signed-off-by: Tom Kerkhove <[email protected]>
@tomkerkhove tomkerkhove requested a review from a team November 25, 2021 14:09
@tomkerkhove
Copy link
Contributor Author

I'd love to update the public API docs but would need some more help. The contribution guide mentions to use intellisense, but do you mean start typing and use the autocomplete or does it support this kind of generation?

Signed-off-by: Tom Kerkhove <[email protected]>
Signed-off-by: Tom Kerkhove <[email protected]>
@cijothomas
Copy link
Member

I'd love to update the public API docs but would need some more help. The contribution guide mentions to use intellisense, but do you mean start typing and use the autocomplete or does it support this kind of generation?

There is no public API change in this PR, so no need of public API doc update.

@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #2686 (68761ea) into main (f153cb1) will decrease coverage by 0.02%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2686      +/-   ##
==========================================
- Coverage   82.95%   82.93%   -0.03%     
==========================================
  Files         249      249              
  Lines        8703     8704       +1     
==========================================
- Hits         7220     7219       -1     
- Misses       1483     1485       +2     
Impacted Files Coverage Δ
...mentation/ExportClient/BaseOtlpHttpExportClient.cs 73.91% <0.00%> (ø)
...ementation/ExportClient/OtlpGrpcLogExportClient.cs 0.00% <0.00%> (ø)
...tation/ExportClient/OtlpGrpcMetricsExportClient.cs 0.00% <0.00%> (ø)
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 35.71% <0.00%> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs 65.00% <33.33%> (-3.43%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 76.47% <0.00%> (-5.89%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

@tomkerkhove
Copy link
Contributor Author

I didn't add any additional testing since EventSourceTest_OpenTelemetryProtocolExporterEventSource seems to be handling this but the code coverage is dropping.

Any guidance on the testing side on what I should improve?

@cijothomas cijothomas merged commit 5f5238d into open-telemetry:main Nov 29, 2021
@tomkerkhove tomkerkhove deleted the connection-info branch November 29, 2021 18:16
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.

4 participants