Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat(instrumentation-tls): wrap tls.connect API #447
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
feat(instrumentation-tls): wrap tls.connect API #447
Changes from 7 commits
1ff9f9d
54315fa
326d0a8
dddc0fc
baeb05d
c5d9f44
81bd589
5588ff0
2a5f840
67b52d4
4d9fdad
01c13d4
198dbee
d6a3f50
6b40e75
1353d56
08ac436
62def9f
9e9c55d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are any of these in the Semantic Conventions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I am aware of. I checked the specs and didn't find them there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment that these are not "official"
we need guidance from the spec how to add attributes that are not yet specified. If the spec eventually adds one of these attributes and it isn't compatible with ours that will be a problem.
@open-telemetry/technical-committee any guidance how to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was confused. I thought you talk about the socket events, but you mean the TLSAttributes? I actually opened an issue already: open-telemetry/opentelemetry-specification#1652
Yes, I can do that.
I guess they are not the only candidates for that, e.g. the DNS package puts the name that was looked up into net.peer.name, which is probably not correct and their might be a semantic convention for DNS.
Since this is kind of related to open-telemetry/opentelemetry-js#2123: Would it help if I scan js and js-contrib (semi)automatically for attributes that are either in the semantic-specs already or that might be candidates for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dyladan I found a bunch of them. There's four different cases:
Since this is a discussion unrelated to this ticket, can you give me some guidance where to move this discussion? Open another issue with the specification repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would open an issue on this repo to track it at least. If you think some should be spec'd then you should open an issue on the spec repo too and link them.