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

Change http.server.active_requests from asynchronous to synchronous UpDownCounter #2451

Closed
wants to merge 1 commit into from

Conversation

trask
Copy link
Member

@trask trask commented Mar 28, 2022

Changes

Changes http.server.active_requests from an asynchronous UpDownCounter to a synchronous UpDownCounter.

Since instrumentation will typically increment/decrement this metric for each request (especially important in order to record its attributes), as opposed to reading the value from an underlying component.

@trask trask force-pushed the http-server-active-requests branch from 2c07271 to 1b391ed Compare March 28, 2022 22:18
@trask trask marked this pull request as ready for review March 28, 2022 22:18
@trask trask requested review from a team March 28, 2022 22:18
@reyang reyang added the area:semantic-conventions Related to semantic conventions label Mar 30, 2022
@reyang
Copy link
Member

reyang commented Mar 30, 2022

Since instrumentation will typically increment/decrement this metric for each request (especially important in order to record its attributes), as opposed to reading the value from an underlying component.

@trask do you think we need both? For example, it's possible that the HTTP server is already running for a period of time before instrumentation is enabled, and if this happened, there is no easy way to know the current number of in-flight requests if the sync version of UpDownCounter is used.

@reyang
Copy link
Member

reyang commented Mar 30, 2022

IIRC there has been discussion regarding how to model the in-flight requests.
One possible way is to have two separate instruments - one for the Total Number of Received Requests and another for Total Number of Finished Requests.
I think we need to gather more perspectives, if this PR is not getting sufficient attention in a week, perhaps bring it to the spec meeting.

@reyang reyang added the spec:metrics Related to the specification/metrics directory label Mar 30, 2022
@anuraaga
Copy link
Contributor

Discussed this a bit in the JVM metrics sig, our feeling is that there doesn't seem to be a need to describe asynchronous or synchronous in the semantic conventions as whether to use one or the other is an implementation detail of the instrumentation, not semantic meaning of the data itself. There is also an idea that synchronous is the "more powerful" instrument so should be preferred where possible, but can't always be used. A synchronous counter with delta temporality can provide more fine grained data over a collecting interval, while cumulative temporality would be the same information effectively as an asynchronous counter.

As an example, for JVM metrics we have two sources for information, JFR (only newer JVMs) and JMX. JFR reports events so e.g., cpu time can be recorded with synchronous instruments, but JMX will only present aggregated data for the same metric that can only be reported with asynchronous instruments. It seems fine for the JFR version to use synchronous and JMX asynchronous for the exact same semantic convention.

@fstab
Copy link
Member

fstab commented Mar 30, 2022

@anuraaga I opened PR #2458. If we don't want to specify whether JVM metrics must be synchronous or asynchronous we should remove "asynchronous" from the current semantic conventions for JVM runtimes.

@anuraaga
Copy link
Contributor

Thanks @fstab - to clarify, I have a feeling this applies to any convention not just JVM. But either way is good to start somewhere

@jmacd
Copy link
Contributor

jmacd commented Mar 30, 2022

I support removing async/sync from all conventions.

@trask
Copy link
Member Author

trask commented Mar 30, 2022

awesome, thanks all! I will close this and send a new PR to remove async/sync from all metric conventions

@trask trask closed this Mar 30, 2022
@trask trask deleted the http-server-active-requests branch March 30, 2022 18:02
@trask
Copy link
Member Author

trask commented Mar 30, 2022

@fstab instead of opening a new PR, I will send a PR to your PR to increase scope of #2458 to include all metric conventions

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 spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants