botocore: sns set destination name attribute only if not phone number

[emdneto]

instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_sns.py::TestSnsExtension::test_publish_to_phone_number 
-------------------------------------------------------------------------------- live log call --------------------------------------------------------------------------------
WARNING  opentelemetry.attributes:__init__.py:100 Invalid type NoneType for attribute 'messaging.destination.name' value. Expected one of ['bool', 'str', 'bytes', 'int', 'float'] or a sequence of those types

[xrmx]

There's also the same TODOs in the tests in opentelemetry-instrumentation-botocore/tests/test_botocore_sns.py

[xrmx]

Maybe the issue here is that this should be set to destination_namewe got from cls._extract_destination_name a few lines above instead?

[emdneto]

In that case, destination_name would be the phone number. I'm not sure if it's a "valid" destination name. According to the semantic convention, it should be used for ARNs, topic /queue names, or something like that 🤔 So I'm presuming if it's phone number there's no destination name

[emdneto]

Wait, I think we are being redundant here no?

MESSAGING_DESTINATION and MESSAGING_DESTINATION_NAME are the same thing

[emdneto]

are you ok to drop L82 and keep only attributes[SpanAttributes.MESSAGING_DESTINATION_NAME] = destination_name ?

[xrmx]

That would break users though. Until we implement the stable opt-in config I would keep the redundancy. Said that there's another issue there we should not publish the phone number because it's PII. So I would reuse the same variable but redact the phone number in _extract_destination_name. This way we can use it directly in the span name.

[xrmx]

Something like this:

diff --git a/instrumentation/opentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/extensions/sns.py b/instrumentation/opentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/extensions/sns.py
index 11761360..26388df4 100644
--- a/instrumentation/opentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/extensions/sns.py
+++ b/instrumentation/opentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/extensions/sns.py
@@ -81,14 +81,8 @@ class _OpPublish(_SnsOperation):
             MessagingDestinationKindValues.TOPIC.value
         )
         attributes[SpanAttributes.MESSAGING_DESTINATION] = destination_name
-
-        # TODO: Use SpanAttributes.MESSAGING_DESTINATION_NAME when opentelemetry-semantic-conventions 0.42b0 is released
-        attributes["messaging.destination.name"] = cls._extract_input_arn(
-            call_context
-        )
-        call_context.span_name = (
-            f"{'phone_number' if is_phone_number else destination_name} send"
-        )
+        attributes[SpanAttributes.MESSAGING_DESTINATION_NAME] = destination_name
+        call_context.span_name = f"{destination_name} send"
 
     @classmethod
     def _extract_destination_name(
@@ -101,7 +95,8 @@ class _OpPublish(_SnsOperation):
         if cls._phone_arg_name:
             phone_number = call_context.params.get(cls._phone_arg_name)
             if phone_number:
-                return phone_number, True
+                # phone number redacted because it's a PII
+                return "phone_number", True
 
         return "unknown", False

Hopefully people won't have topics called "phone_number" :)

[emdneto]

Good catch 👍🏻