Skip to content

fix: end timer on stream end or error #503

Merged
wkillerud merged 2 commits intomainfrom
stream-metrics
Aug 22, 2025
Merged

fix: end timer on stream end or error #503
wkillerud merged 2 commits intomainfrom
stream-metrics

Conversation

@wkillerud
Copy link
Copy Markdown
Contributor

@wkillerud wkillerud commented Aug 21, 2025

We measure the action as done when the stream is returned, not when the stream itself is finished. This could be misleading.

See the difference between END and FINISH.

# Subtest: test/handlers/pkg.get.js
    END 2181.057062
    FINISH 12189.26404
    # Subtest: pkg.get() - URL parameters is URL encoded
        ok 1 - should respond with expected status code
        ok 2 - should be possible to retrieve a payload when handlers values is URL encoded
        1..2
    ok 1 - pkg.get() - URL parameters is URL encoded # time=10025.446ms
    
    1..1

@wkillerud
Copy link
Copy Markdown
Contributor Author

You could argue that the sink should be responsible for measuring its own duration, and that this is working as intended as a "stream start" timing.

We measure the action as done when the stream is returned, not when the
stream itself is finished. This could be misleading.
This makes the timing metrics match expectations and lets us know of
slowness between Eik and the sink.
@wkillerud wkillerud marked this pull request as ready for review August 22, 2025 08:29
@wkillerud wkillerud changed the title test: demonstrate timing issue with histogram fix: end timer on stream end or error Aug 22, 2025
@leftieFriele leftieFriele self-requested a review August 22, 2025 08:33
Copy link
Copy Markdown

@leftieFriele leftieFriele left a comment

Choose a reason for hiding this comment

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

👍 I think this change makes sense, as you would expect this metrics to measure the time it takes for an asset to be fetched. The time for it to start really isn't interesting, as that would mean we are measuring something internal in the client.

@wkillerud wkillerud merged commit 5cea253 into main Aug 22, 2025
9 checks passed
@wkillerud wkillerud deleted the stream-metrics branch August 22, 2025 12:12
github-actions Bot pushed a commit that referenced this pull request Aug 22, 2025
## [2.0.22](v2.0.21...v2.0.22) (2025-08-22)

### Bug Fixes

* end timer on stream end or error  ([#503](#503)) ([5cea253](5cea253))
@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 2.0.22 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants