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: create XRayInstrument #16

Open
wants to merge 52 commits into
base: main
Choose a base branch
from
Open

feat: create XRayInstrument #16

wants to merge 52 commits into from

Conversation

pokryfka
Copy link
Owner

No description provided.

Copy link
Contributor

@slashmo slashmo left a comment

Choose a reason for hiding this comment

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

Thanks again for creating this PR, looks great already 👍
I've added some comments inline.

Sources/AWSXRayInstrument/Instrument.swift Outdated Show resolved Hide resolved
Sources/AWSXRayInstrument/Instrument.swift Outdated Show resolved Hide resolved
Sources/AWSXRayInstrument/Span.swift Outdated Show resolved Hide resolved
Sources/AWSXRayInstrument/Span.swift Outdated Show resolved Hide resolved
Sources/AWSXRayInstrument/Span.swift Outdated Show resolved Hide resolved
@pokryfka
Copy link
Owner Author

pokryfka commented Jul 20, 2020

@slashmo thank you for the comments and snippets, I will copy that and commit. feel welcome to push changes, just until it works more or less and tests pass (those I created fail miserably lol) let's consider feature/instrument the integration branch for the instrument related changes.

The refactoring changes I noted in my head yesterday:

  • expose subsegment name (currently internal)
  • define Context type, this will be equivalent of the TracingHeader but it feels weird to use "TracingHeader" in Segment
  • add attribute of Context type to subsegment (will contain traceId, optionally parent and samplng decision which are separate attributes at the moment; the sampling decision is related with isRecording defined in Span)
  • the Context type should eventually be the BaggageContext though it may be in 2nd iteration of changes
  • extend annotations and metadata interface to let remove existing values to match SpanAttributes

@pokryfka
Copy link
Owner Author

well, the tests defined yesterday pass now - so its a small progress ;-)
there are more tests which, once defined, will fail though

@pokryfka pokryfka closed this Jul 21, 2020
@pokryfka pokryfka reopened this Jul 21, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2020

Codecov Report

Merging #16 into master will increase coverage by 0.64%.
The diff coverage is 91.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   77.11%   77.76%   +0.64%     
==========================================
  Files          29       31       +2     
  Lines        1219     1277      +58     
==========================================
+ Hits          940      993      +53     
- Misses        279      284       +5     
Impacted Files Coverage Δ
Sources/AWSXRayInstrument/Span.swift 89.36% <89.36%> (ø)
Sources/AWSXRayInstrument/Instrument.swift 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c505ac2...e09bc8e. Read the comment docs.

@pokryfka
Copy link
Owner Author

@ktoso @slashmo first version with Baggage, please let me know if it makes sense, will highlight specific questions/issues in the code

out now, will recheck it in the evening as well as pull in any related changes from gsoc-swift-tracing

@pokryfka
Copy link
Owner Author

pokryfka commented Jul 25, 2020

@slashmo @ktoso

I deployed a "hello world" lambda a few weeks ago when testing XRayRecorder;
it creates a few subsegments:

result from AWSXRayInstrumentExample (which called the url above using BetterHTTPClient):

Screen Shot 2020-07-25 at 16 08 19

note that currently events are recorded as both subsegments and metadata:

Screen Shot 2020-07-25 at 16 17 58

Screen Shot 2020-07-25 at 16 12 46

@slashmo
Copy link
Contributor

slashmo commented Jul 25, 2020

there is "hello world" lambda I deployed which creates a few subsegments:

Wow, that's awesome! 🎉

@ktoso
Copy link

ktoso commented Jul 27, 2020

Ah thanks a lot, that explains how metadata is presented. I guess yeah events in there sound like an okey way to at least show them to users somehow 👍

@pokryfka pokryfka mentioned this pull request Jul 30, 2020
9 tasks
@pokryfka pokryfka moved this from In progress to To do in aws-xray-sdk-swift Jul 30, 2020
@pokryfka pokryfka removed the enhancement New feature or request label Jul 31, 2020
@pokryfka pokryfka removed this from To do in aws-xray-sdk-swift Jul 31, 2020
@pokryfka pokryfka mentioned this pull request Aug 1, 2020
10 tasks
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.

None yet

4 participants