search

embrace-io / embrace-apple-sdk

Embrace's Swift SDK built on OpenTelemetry
https://embrace.io/docs/ios/
Apache License 2.0
117 stars 11 forks source link

Fix recordCompletedSpan to set Span.Status consistently with other `end` methods #46

Closed atreat closed 3 months ago

atreat commented 3 months ago

Updates recordCompletedSpan to use the end(errorCode:time:) method and properly set Span.Status

Status will be OK if errorCode is nil, otherwise it will be ERROR.

Fixes duplication of SpanErrorCode and ErrorCode definitions

github-actions[bot] commented 3 months ago

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

github-actions[bot] commented 3 months ago
Warnings
:warning: No CHANGELOG entry added.

Generated by :no_entry_sign: Danger Swift against 8197b80b480aa535462e81e5f11ec069665e7b63

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 94.52055% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.11%. Comparing base (2ec8b91) to head (8197b80). Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...port/Mocks/SpanExporter/InMemorySpanExporter.swift 75.00% 2 Missing :warning:
...aceSemantics/SpanBuilder/SpanBuilder+Embrace.swift 0.00% 1 Missing :warning:
Tests/TestSupport/XCTSkip+Helpers.swift 75.00% 1 Missing :warning:
Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46/graphs/tree.svg?width=650&height=150&src=pr&token=R3KMG2Ar52&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io)](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io) ```diff @@ Coverage Diff @@ ## main #46 +/- ## ========================================== + Coverage 91.78% 92.11% +0.32% ========================================== Files 389 391 +2 Lines 18690 18749 +59 ========================================== + Hits 17154 17270 +116 + Misses 1536 1479 -57 ``` | [Files with missing lines](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io) | Coverage Δ | | |---|---|---| | [Sources/EmbraceCore/Public/Embrace+OTel.swift](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&filepath=Sources%2FEmbraceCore%2FPublic%2FEmbrace%2BOTel.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-U291cmNlcy9FbWJyYWNlQ29yZS9QdWJsaWMvRW1icmFjZStPVGVsLnN3aWZ0) | `79.45% <100.00%> (+10.95%)` | :arrow_up: | | [...nal/Trace/EmbraceSemantics/Span/Span+Embrace.swift](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&filepath=Sources%2FEmbraceOTelInternal%2FTrace%2FEmbraceSemantics%2FSpan%2FSpan%2BEmbrace.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-U291cmNlcy9FbWJyYWNlT1RlbEludGVybmFsL1RyYWNlL0VtYnJhY2VTZW1hbnRpY3MvU3Bhbi9TcGFuK0VtYnJhY2Uuc3dpZnQ=) | `70.83% <100.00%> (+58.33%)` | :arrow_up: | | [...Trace/EmbraceSemantics/Span/SpanData+Embrace.swift](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&filepath=Sources%2FEmbraceOTelInternal%2FTrace%2FEmbraceSemantics%2FSpan%2FSpanData%2BEmbrace.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-U291cmNlcy9FbWJyYWNlT1RlbEludGVybmFsL1RyYWNlL0VtYnJhY2VTZW1hbnRpY3MvU3Bhbi9TcGFuRGF0YStFbWJyYWNlLnN3aWZ0) | `93.33% <100.00%> (ø)` | | | [...ntegrationTests/Embrace+OTelIntegrationTests.swift](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&filepath=Tests%2FEmbraceCoreTests%2FIntegrationTests%2FEmbrace%2BOTelIntegrationTests.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-VGVzdHMvRW1icmFjZUNvcmVUZXN0cy9JbnRlZ3JhdGlvblRlc3RzL0VtYnJhY2UrT1RlbEludGVncmF0aW9uVGVzdHMuc3dpZnQ=) | `100.00% <100.00%> (ø)` | | | [...acer/Span/Processor/SingleSpanProcessorTests.swift](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&filepath=Tests%2FEmbraceOTelInternalTests%2FTrace%2FTracer%2FSpan%2FProcessor%2FSingleSpanProcessorTests.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-VGVzdHMvRW1icmFjZU9UZWxJbnRlcm5hbFRlc3RzL1RyYWNlL1RyYWNlci9TcGFuL1Byb2Nlc3Nvci9TaW5nbGVTcGFuUHJvY2Vzc29yVGVzdHMuc3dpZnQ=) | `98.96% <100.00%> (ø)` | | | [...aceSemantics/SpanBuilder/SpanBuilder+Embrace.swift](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&filepath=Sources%2FEmbraceOTelInternal%2FTrace%2FEmbraceSemantics%2FSpanBuilder%2FSpanBuilder%2BEmbrace.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-U291cmNlcy9FbWJyYWNlT1RlbEludGVybmFsL1RyYWNlL0VtYnJhY2VTZW1hbnRpY3MvU3BhbkJ1aWxkZXIvU3BhbkJ1aWxkZXIrRW1icmFjZS5zd2lmdA==) | `22.22% <0.00%> (ø)` | | | [Tests/TestSupport/XCTSkip+Helpers.swift](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&filepath=Tests%2FTestSupport%2FXCTSkip%2BHelpers.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-VGVzdHMvVGVzdFN1cHBvcnQvWENUU2tpcCtIZWxwZXJzLnN3aWZ0) | `75.00% <75.00%> (ø)` | | | [...port/Mocks/SpanExporter/InMemorySpanExporter.swift](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46?src=pr&el=tree&filepath=Tests%2FTestSupport%2FMocks%2FSpanExporter%2FInMemorySpanExporter.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-VGVzdHMvVGVzdFN1cHBvcnQvTW9ja3MvU3BhbkV4cG9ydGVyL0luTWVtb3J5U3BhbkV4cG9ydGVyLnN3aWZ0) | `70.58% <75.00%> (ø)` | | ... and [11 files with indirect coverage changes](https://app.codecov.io/gh/embrace-io/embrace-apple-sdk/pull/46/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io)
atreat commented 3 months ago

if somebody creates the span using the SpanBuilder (aka buildSpan public API), would they be able to use the end(errorCode:time:) w/o importing EmbraceOTelInternal or shall we suggest they set the status manually before ending?

I would not expect a caller to need to import EmbraceOTelInternal. I'll make sure that is not required. We can also look into setting the status in the SpanProcessor instead to move this logic from the interface layer

atreat commented 3 months ago

I would not expect a caller to need to import EmbraceOTelInternal

This is necessary right now. Going to see what I can do to prevent this from being necessary

ArielDemarco commented 3 months ago

I would not expect a caller to need to import EmbraceOTelInternal

This is necessary right now. Going to see what I can do to prevent this from being necessary

It's good to know. If you see the solution would take longer, we can address it in another PR

atreat commented 3 months ago

Going to address the EmbraceOtelInternal import requirement in another PR.