NH-30870 calculate_attributes adds existing attrs earlier

[tammy-baylis-swi]

Bugfix: custom sampler's calculate_attributes now adds existing attributes earlier. This is important for scenarios where the customer uses the SDK to start spans with custom attributes, e.g.

  with tracer.start_as_current_span(
      "manual_instrumentation_django_a",
      attributes={"foo-django-a": "bar-django-a"},
  ):
    # do stuff

Example trace with this fix: https://my.na-01.st-ssp.solarwinds.com/136444677275740160/traces/6D23EB224226CC4A59FA70D95C0BB1BE/892E8BFFDE370C5D/details/789BCA3EB9F3A9A0/rawData which was created using this WIP testbed update: https://github.com/appoptics/solarwinds-apm-python-testbed/pull/45

Add a unit tests. All are passing.

[cheempz]

Thanks for catching this @tammy-baylis-swi! The fix itself works, but has opened a small can of worms in my mind on how it relates to the setting of our sw.tracestate_parent_id attribute ;p

[cheempz]

I'm wondering now why this else is predicated on whether there is attributes... is it because we're using the presence of attributes to determine that "this is a root span for the service" (i.e. root span with no parent context or with a remote parent context) -- to just set the tracestate attr once per trace?

[tammy-baylis-swi]

I was hoping you knew, and the unit tests around this are so brittle. 🙃 The presence of attributes to check for that kinda makes sense, but why wouldn't it it just check for those contexts instead? If it was younger Tammy writing her very first sample that could be an excuse... I'll look more into this!

[tammy-baylis-swi]

In b56c871, c26c7ea I've changed the condition to add sw.w3c.tracestate to attributes if the value is available from the parent and the parent is_remote, not based on existing attributes. This I think better matches scenario 5 in W3C Acceptance Criteria:

service root span reports KV sw.w3c.tracestate set to value of the inbound tracestate header as-is

Unit tests all pass. Trace still exports (example), though it's through ConsoleSpanExporter that I could see that the server span for Django B's home_b/ route is the only one in the Django A-B distributed trace that has sw.w3c.tracestate set (e.g. attributes > "sw.w3c.tracestate": "sw=f452fc038dc44249-01").

As to why this has been based on injected/existing attributes this whole time, I couldn't find it in my old manual test docs!

[cheempz]

Minor, if attributes is passed directly from the start trace --> sampler --> here, it seems it would only carry attributes set either by an instrumentation library, or by a manual SDK call. Not sure if there would be other vendor info?

[tammy-baylis-swi]

Yes! Fixed the comment in 4fd8050

[tammy-baylis-swi]

Thanks @cheempz for checking the fix and asking about that weeeeird sw.w3c.tracestate sw.tracestate_parent_id condition! Please could you have another look when you have a moment.

[tammy-baylis-swi]

Hi again @cheempz ! Thank you for pointing out my wording of sw.w3c.tracestate instead of sw.tracestate_parent_id -- yikes!

As we chatted, the logic was still correct in that sw.tracestate_parent_id is set as the parent span ID if it exists. I've enhanced some unit tests in b52a857, and updated comment and changelog in 9585d5c and 5b99a1c. Also adjusted logging in f4344a3.

Please let me know what you think this time!

[cheempz]

👍 thanks for the revisit @tammy-baylis-swi!