Skip to content

Add Semantic conventions for TLS/SSL encrypted communication#2992

Closed
svrnm wants to merge 43 commits intoopen-telemetry:mainfrom
svrnm:add-tls-semconv
Closed

Add Semantic conventions for TLS/SSL encrypted communication#2992
svrnm wants to merge 43 commits intoopen-telemetry:mainfrom
svrnm:add-tls-semconv

Conversation

@svrnm
Copy link
Copy Markdown
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 requested review from a team November 30, 2022 13:55
@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
@svrnm
fix toc for tls.md
0ccc805 
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
Copy Markdown

github-actions Bot commented Dec 14, 2022

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
Copy Markdown
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
Copy Markdown

github-actions Bot commented Dec 28, 2022

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 28, 2022
@github-actions
Copy link
Copy Markdown

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
Copy Markdown
Member Author

svrnm commented Jan 9, 2023

anything missing, any additional feedback?

@github-actions github-actions Bot removed the Stale label Jan 10, 2023
joaopgrassi
joaopgrassi reviewed Jan 10, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Please also add a changelog entry: https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md?plain=1#L29

Comment thread semantic_conventions/trace/tls.yaml Outdated
@github-actions github-actions Bot added the Stale label Apr 11, 2023
@joaopgrassi joaopgrassi removed the Stale label Apr 11, 2023
@joaopgrassi
Copy link
Copy Markdown
Member

joaopgrassi commented Apr 18, 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

@svrnm
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
joaopgrassi
joaopgrassi approved these changes May 3, 2023
View reviewed changes
Copy link
Copy Markdown
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

opentelemetry-specification/semantic_conventions/resource/cloud_provider/aws/logs.yaml

Line 9 in abf540c

type: string[]
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?

Comment thread semantic_conventions/trace/tls.yaml Outdated
Comment thread specification/trace/semantic_conventions/tls.md Outdated
Comment thread CHANGELOG.md Outdated
@svrnm
Copy link
Copy Markdown
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

opentelemetry-specification/semantic_conventions/resource/cloud_provider/aws/logs.yaml

Line 9 in abf540c

type: string[]

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
Copy Markdown
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
Copy Markdown
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!

jsuereth
jsuereth requested changes May 12, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Please move this to https://github.com/open-telemetry/semantic-conventions

@svrnm svrnm mentioned this pull request May 15, 2023
Merged
@svrnm
Copy link
Copy Markdown
Member Author

svrnm commented May 15, 2023

moved to https://github.com/open-telemetry/semantic-conventions

@svrnm svrnm closed this May 15, 2023
@svrnm svrnm deleted the add-tls-semconv branch January 11, 2024 14:38
@sandy2008 sandy2008 mentioned this pull request Mar 23, 2026
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsuereth jsuereth jsuereth requested changes

@joaopgrassi joaopgrassi joaopgrassi approved these changes

+1 more reviewer

@yurishkuro yurishkuro yurishkuro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:semantic-conventions Related to semantic conventions semconv:HTTP spec:trace Related to the specification/trace directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add semantic convention for describing SSL/TLS connections

9 participants

@svrnm @joaopgrassi @trask @reyang @jsuereth @yurishkuro @carlosalberto @arminru @SergeyKanzhelev