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 #2992

Closed
wants to merge 43 commits into from

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Nov 30, 2022

Fixes #1652

Changes

Add trace/span semantic conventions for TLS/SSL.

I created a similar PR (#1854) a long while back that stalled eventually, I wanted to give it another try. Before doing so I did some scrubbing, added some references, removed some things where I was uncertain and updated a few things to match the APIs or RFCs (e.g. "not_before" and "not_after" are used for validity instead of valid_from and valid_to)

Reference implementation: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-net/src/instrumentation.ts#L121

References:

@svrnm svrnm changed the title Add tls semconv @svrnm Add Semantic conventions for TLS/SSL encrypted communication Nov 30, 2022
@svrnm svrnm changed the title @svrnm Add Semantic conventions for TLS/SSL encrypted communication Add Semantic conventions for TLS/SSL encrypted communication Nov 30, 2022
Signed-off-by: svrnm <[email protected]>
@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory semconv:HTTP labels Dec 5, 2022
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 14, 2022
@svrnm
Copy link
Member Author

svrnm commented Dec 19, 2022

@open-telemetry/specs-approvers any feedback?:)

@github-actions github-actions bot removed the Stale label Dec 20, 2022
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 6, 2023
@svrnm
Copy link
Member Author

svrnm commented Jan 9, 2023

anything missing, any additional feedback?

@github-actions github-actions bot removed the Stale label Jan 10, 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.

semantic_conventions/trace/tls.yaml Outdated Show resolved Hide resolved
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@joaopgrassi
Copy link
Member

Now that the ECS OTEP was merged, do we also want maybe to adopt the attributes from there for these? https://www.elastic.co/guide/en/ecs/current/ecs-tls.html

@svrnm
Copy link
Member Author

svrnm commented Apr 24, 2023

Now that the ECS OTEP was merged, do we also want maybe to adopt the attributes from there for these? https://www.elastic.co/guide/en/ecs/current/ecs-tls.html

@joaopgrassi
makes sense to me, what would need to be done for that?

@svrnm svrnm requested a review from a team April 24, 2023 07:24
@trask
Copy link
Member

trask commented Apr 25, 2023

Now that the ECS OTEP was merged, do we also want maybe to adopt the attributes from there for these? https://www.elastic.co/guide/en/ecs/current/ecs-tls.html

@joaopgrassi

makes sense to me, what would need to be done for that?

@joaopgrassi thx for bringing that up here!

@svrnm check out open-telemetry/semantic-conventions#1012 and #3409 for some examples, and see if it makes sense to rename the new attributes you've proposed to align with the ECS names.

We will bring over ECS attributes more proactively in the future, but for now we just want to avoid new divergence in cases where alignment makes sense.

@svrnm
Copy link
Member Author

svrnm commented Apr 25, 2023

@joaopgrassi , @trask thank you!

I gave it a try, a few notable changes:

  • ECS does not use "peer" and "host" but "server" and "client", I used the first initially to be consistent with net.*, but doing some reading on TLS the use of "server" and "client" is more reasonable, because they have distinct meaning and distinct properties in a secure communication
  • I skipped some attributes from ECS for now, namely because they require arrays, which I was not sure if we have that already in the SemConv
  • The hashes are formatted differently, I picked the one of ECS over the one suggested here initially

@github-actions
Copy link

github-actions bot commented May 3, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 3, 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.

LGTM, thanks for putting the effort on this!

About the array attributes, I think we should be OK in adding them, as there's already some conventions using it like

and also HTTP headers.

I'm not an expert in this, so I don't know how crucial those array attributes are, but I guess the goal is to take over all ECS attributes. @trask @AlexanderWert what do you think?

semantic_conventions/trace/tls.yaml Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/tls.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@svrnm
Copy link
Member Author

svrnm commented May 4, 2023

LGTM, thanks for putting the effort on this!

About the array attributes, I think we should be OK in adding them, as there's already some conventions using it like

and also HTTP headers.
I'm not an expert in this, so I don't know how crucial those array attributes are, but I guess the goal is to take over all ECS attributes. @trask @AlexanderWert what do you think?

I added certificate and certificate_chain now, please give it a look.

@github-actions github-actions bot removed the Stale label May 5, 2023
@reyang
Copy link
Member

reyang commented May 9, 2023

@svrnm heads up - most likely this PR will be closed, and we'll ask you to resubmit the PR in a new repo, please refer to #3474 (comment).

@svrnm
Copy link
Member Author

svrnm commented May 11, 2023

@svrnm heads up - most likely this PR will be closed, and we'll ask you to resubmit the PR in a new repo, please refer to #3474 (comment).

I assumed that, let me find some time to bring it over to the new semconv repo!

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

@svrnm
Copy link
Member Author

svrnm commented May 15, 2023

@svrnm svrnm closed this May 15, 2023
@svrnm svrnm deleted the add-tls-semconv branch January 11, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions semconv:HTTP spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add semantic convention for describing SSL/TLS connections
9 participants