Add Semantic conventions for TLS/SSL encrypted communication

[svrnm]

Replacement for open-telemetry/opentelemetry-specification#2992

[joaopgrassi]

We will need to add a changelog entry once #18 is merged

[svrnm]

I changed the PR to fit into the new structure of the Semantic Conventions.

As discussed before the thing I am completely stuck on is the question how TLS/SSL should be represented, as there are multiple ways to go about it.

[AlexanderWert]

protocol.name / protocol.version need to be swapped.

Nit: Also, can we order the attributes within a namespace alphabetically? In namespaces with many attributes (as it is the case here) it's otherwise difficult to keep orientation.

[lmolkova]

@svrnm do you want to continue working on this?

We're adding tls.protocol.name and tls.protocol.version in #283. If you want to bake this one for longer, would you mind if I go ahead and send another PR to add these two attributes only?

[AlexanderWert]

In general looks good!

One proposal, though:

Now, that we have the registry directory I would propose to move all of this into the attributes registry first.

This PR is not defining much around the usage of the attributes anyways, but rather introduces the attributes as such.

I'd propose to ...

• ... move the YAML file into /model/registry/tls.yaml (and flatten it's structure into a single attribute group)
  • please also remove all the requirement levels (the different semantic conventions that will use these attributes will overwrite the default if needed)
• ... move the markdown file to /docs/attributes-registry/tls.md
  • just render the attributes table in there (this PR does not provide additional specification on the usage anyways)

[svrnm]

@svrnm do you want to continue working on this?

We're adding tls.protocol.name and tls.protocol.version in #283. If you want to bake this one for longer, would you mind if I go ahead and send another PR to add these two attributes only?

@lmolkova I would have no issue to trim down this PR to a minimal set of attributes to speed up the process of having it merged, even if it is only two. I extended this PR by many many attributes because it was specificially asked for to have it compatible with ECS.

My understanding is that right now the issue is not with the attributes and how many there are, but that it is not yet clear how TLS is represented (see this discussion: #21 (comment)):

if we look at existing spans representing networking requests (e.g. HTTP), we only have a single span for the top level communication (here HTTP), while there is no parent span representing the TCP connection & it's related timing (or the DNS request that may be happening within the HTTP call to resolve the remote host). Attributes related to the TCP connection (server., `network. ) are "bubbled" up to the HTTP connection

TLS/SSL is very similar to that. So my question is now, if this is by design and there is a section in the spec calling this out or if this is not yet specified? If this is already defined, I think TLS should be treated similarly.

If #283 creates some clarity around that, what would stop us from merging the whole PR?

[svrnm]

Thanks @AlexanderWert , let me look into that

[svrnm]

@AlexanderWert please take another look if what I did makes sense so far.

Note, that I also have used a small script to sort the attributes alphabetically:

#!/bin/bash

# Read the YAML file path from the first argument
yaml_file="$1"

# Read the YAML data
yaml_data=$(cat "$yaml_file")

# Sort attributes alphabetically by id
sorted_yaml=$(echo "$yaml_data" | yq eval '
    .groups[].attributes |= sort_by(.id)
' -)

# Write the sorted YAML back to the file
echo "$sorted_yaml" > "$yaml_file"

If this might be valuable to others as well I can check it into internal/tools/sort_attributes.sh or other suggested places (full disclosure: I asked a chat bot to generate that script snippet)

[AlexanderWert]

LGTM

[trisch-me]

My understanding is that right now the issue is not with the attributes and how many there are, but that it is not yet clear how TLS is represented (see this discussion: #21 (comment)):

I think once we have moved namespace to the registry, this question could stay out of this PR and could be addressed separately.

[trisch-me]

@svrnm I think if you will fix failing check and update readme.md inside attributes-registry we could finally merge this PR

[AlexanderWert]

@lmolkova @jsuereth Now, that the proposed attributes here are only being defined in the registry (and not used in concrete semconv) are you fine with the proposal here?

Things changed, I will take another look.

[joaopgrassi]

@svrnm

Note, that I also have used a small script to sort the attributes alphabetically:

The build-tools should now do this OOTB. Did you still use your script? If yes, it may be a good idea to run the tool again to see if it changes from your script, otherwise changes to the yaml later may change the diff in the markdown.

[svrnm]

@svrnm

Note, that I also have used a small script to sort the attributes alphabetically:

The build-tools should now do this OOTB. Did you still use your script? If yes, it may be a good idea to run the tool again to see if it changes from your script, otherwise changes to the yaml later may change the diff in the markdown.

Oh good to know! I ran make once again and no changes were needed, so it looks like all went well?

[svrnm]

that came a long way, thank you all! :-)