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.
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
Enhance semantic conventions for HTTP #263
Enhance semantic conventions for HTTP #263
Changes from 27 commits
dee825c
6d4ed7f
b894d0b
5621102
417f488
b6e79ba
a452423
ab163c9
6be7dc1
af08156
d362e39
f4d69c2
673635c
471aca3
62361d2
6ab5dc9
1f796f3
a9341a9
62a326c
868eaf7
474ebaf
8699757
fd56d2c
7355684
e8d4b81
fcdaead
b51463f
119509e
5f5c4b6
a432832
291ba38
9364357
280bd01
c59d280
8a25ef4
3f055cb
2e5c5eb
a8685ca
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.
I'd suggest to keep it in resource API, not in individual requests
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.
As I understand resources, they are process-wide. However, a process can host multiple apps.
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 created #274 "Allow resources as span attributes"
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 am confused by this multiple "applications". What do you call an "application"?
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.
Maybe the "Definitions" section above is helpful. But you may also consider the examples. The details depend on the technology. To add another example: In Java EE, an app is the entity described by the "web.xml" file and is defined as follows:
(quote from SRV.9 of https://jcp.org/aboutJava/communityprocess/maintenance/jsr053/index2.html)
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.
If we decided that only one http app per process is supported, then the app would probably be already covered by the proposed
service.name
resource (https://github.com/open-telemetry/opentelemetry-specification/pull/303/files). But note that at the time I wrote this PR, there were no semantic conventions on resources at all, and I still think that resources are very vaguely specified, so it's hard for me to say whether something could be a resource or not, when I don't really know what a resource actually is.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 believe that Resource is not process-wide concept. It's per-Tracer concept. Here it says:
So for the case of multiple apps inside the process - each app may initialize it's own
Tracer
with the app-specific properties. Same properties may be interesting for the child spans so ideally this Tracer needs to be shared with all libraries used in this app.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.
(Don't forget that the
Meter
API will need equal treatment as far as resources go.)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.
Thanks, I will look into that! This also seems like something that needs to be reworded with named tracers (probably the Resources would be per TracerFactory).
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'll remove
http.app
for now and created #335 to track that.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.
why
peer.ip
would not take anX-Forwarded-For
into consideration? What would be the reason to have peer.ip carry the information that is meaningless semantically in many cases?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.
The good thing about having that
peer.ip
definition is that it is very simple (defined basically by a call to http://man7.org/linux/man-pages/man2/getpeername.2.html on the network socket) and applicable to all protocols. And it's not meaningless IMHO, as it at least allows you to detect that the request got through a proxy (for example, maybe you notice one day that all requests with a particular peer.ip take longer than other requests because the proxy has network troubles).On the other hand http.client_ip is very http specific. The alternative would be to have peer.ip be defined differently for each type of span, which feels very unclean to me (And what if a span combines e.g. HTTP and some NoSQL DB and both have conflicting definitions of peer.ip? Now you have to pick one.)
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.
so this example is not how span will look like as you either have url or target, correct? Maybe it's worth splitting into two examples to avoid confusion
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.
Good point, I'll do that. Raises the priority of #311 though 😄
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 only included a remark about the
http.url
value below the attribute table and removed the attribute from the table. I also added a client-side example.