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

feat: add request measurement for requests #270

Merged

Conversation

stijnmoreels
Copy link
Member

Use a new RequestMeasurement object to measure requests.

Closes #268

@netlify
Copy link

netlify bot commented Jan 21, 2022

✔️ Deploy Preview for arcus-observability ready!

🔨 Explore the source changes: 730bb27

🔍 Inspect the deploy log: https://app.netlify.com/sites/arcus-observability/deploys/61efa4f6ba241c000856953d

😎 Browse the preview: https://deploy-preview-270--arcus-observability.netlify.app

@stijnmoreels
Copy link
Member Author

Point of notice: the current .LogRequest and its overloads don't use the DepenencyMeasurement, so no obsolete-making was necessary. They use a single TimeSpan for duration and are using the DateTimeOffset.UtcNow internally as start time.

Copy link
Member

@fgheysels fgheysels left a comment

Choose a reason for hiding this comment

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

Great, nice implementation & docs!

I just wonder why the LogHttpRequest methods do take a Timespan as argument instead of a RequestMeasurement ?

@fgheysels fgheysels assigned stijnmoreels and unassigned fgheysels Jan 24, 2022
@stijnmoreels
Copy link
Member Author

Great, nice implementation & docs!

I just wonder why the LogHttpRequest methods do take a Timespan as argument instead of a RequestMeasurement ?

They use DateTimeOffset.UtcNow internally as startTime. This is more a convenience here, bc it allows users not to worry about this. These extensions are for example used in the request middleware of our Web API where can directly call this, and it uses the current time as start time.
Other than Azure Service Bus (where you have to peek/receive the message yourself), this start time can be a little different, so we allow consumers to determine themselves when it should be tracked.

But you're actually right. We could have additional extension methods that takes in this measurement. But since there's nobody yet asking about it, I'm not sure if we should. On the other hand, one could argue if we should then include it here 😉 .

@fgheysels
Copy link
Member

Great, nice implementation & docs!
I just wonder why the LogHttpRequest methods do take a Timespan as argument instead of a RequestMeasurement ?

They use DateTimeOffset.UtcNow internally as startTime. This is more a convenience here, bc it allows users not to worry about this. These extensions are for example used in the request middleware of our Web API where can directly call this, and it uses the current time as start time. Other than Azure Service Bus (where you have to peek/receive the message yourself), this start time can be a little different, so we allow consumers to determine themselves when it should be tracked.

But you're actually right. We could have additional extension methods that takes in this measurement. But since there's nobody yet asking about it, I'm not sure if we should. On the other hand, one could argue if we should then include it here 😉 .

I was just thinking: why not being consistent and use the measurements types there as well. But if you have a good reason we can leave it as is. If we have a request for this, we can add it indeed.

@fgheysels fgheysels merged commit 3ae1b79 into arcus-azure:master Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce a RequestMeasurement which is used to log the duration of a request
2 participants