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

Add Semantic conventions for TLS/SSL encrypted communication #21

Merged
merged 22 commits into from
Nov 13, 2023

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented May 15, 2023

@svrnm svrnm requested review from a team May 15, 2023 08:57
joaopgrassi
joaopgrassi previously approved these changes May 15, 2023
Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

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

@svrnm
Copy link
Member Author

svrnm commented Jul 28, 2023

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.

docs/tls/tls-spans.md Outdated Show resolved Hide resolved
@jsuereth jsuereth added the experts needed This issue or pull request is outside an area where general approvers feel they can approve label Aug 2, 2023
Copy link
Member

@AlexanderWert AlexanderWert left a comment

Choose a reason for hiding this comment

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

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.

model/trace/tls.yaml Outdated Show resolved Hide resolved
model/trace/tls.yaml Outdated Show resolved Hide resolved
@lmolkova
Copy link
Contributor

@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 lmolkova mentioned this pull request Oct 13, 2023
Copy link
Member

@AlexanderWert AlexanderWert left a comment

Choose a reason for hiding this comment

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

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)

docs/tls/tls-spans.md Outdated Show resolved Hide resolved
model/trace/tls.yaml Outdated Show resolved Hide resolved
model/trace/tls.yaml Outdated Show resolved Hide resolved
docs/tls/tls-spans.md Outdated Show resolved Hide resolved
@svrnm
Copy link
Member Author

svrnm commented Oct 16, 2023

@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
Copy link
Member Author

svrnm commented Oct 16, 2023

Thanks @AlexanderWert , let me look into that

@svrnm
Copy link
Member Author

svrnm commented Oct 16, 2023

@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)

Signed-off-by: svrnm <[email protected]>
Copy link
Member

@AlexanderWert AlexanderWert left a comment

Choose a reason for hiding this comment

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

LGTM

model/registry/tls.yaml Outdated Show resolved Hide resolved
@trisch-me
Copy link
Contributor

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
Copy link
Contributor

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

@AlexanderWert
Copy link
Member

@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?

@joaopgrassi joaopgrassi dismissed their stale review October 27, 2023 14:22

Things changed, I will take another look.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Member Author

svrnm commented Nov 8, 2023

@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?

@AlexanderWert AlexanderWert merged commit 659f03c into open-telemetry:main Nov 13, 2023
9 checks passed
@svrnm svrnm deleted the add-tls-semconv branch November 13, 2023 14:33
@svrnm
Copy link
Member Author

svrnm commented Nov 13, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experts needed This issue or pull request is outside an area where general approvers feel they can approve
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants