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

LogRequest is not logging the correct start-time #356

Closed
fgheysels opened this issue Mar 24, 2022 · 1 comment · Fixed by #357
Closed

LogRequest is not logging the correct start-time #356

fgheysels opened this issue Mar 24, 2022 · 1 comment · Fixed by #357
Assignees
Labels
application-insights All issues related to Azure Application Insights enhancement New feature or request requests All issues related to requests
Milestone

Comments

@fgheysels
Copy link
Member

This is related to arcus-azure/arcus.webapi#310

When calling LogRequest, the timestamp at which the request was started, is not correctly logged. (See the related issue for more detailed information).

This is because the LogRequest method does not contain a parameter that represents the startDateTime, and further up the chain, DateTimeOffset.UtcNow is used for determining the start-date of the request, which is obviously wrong, as the time at which you call this LogRequest method, is not necessarily at the time the request starts.

To fix this, we need to pass in the correct request-starttime to the LogRequest method. There are different approaches for this:

  • add yet another parameter to the LogRequest method which represents the start-time of the request. Drawback of this approach is that the method signature becomes bloated. Existing LogRequest overloads can call the newly introduced version and pass in DateTimeOffset.UtcNow as start-time. However, these existing methods should be marked as obsolete imho.

  • Instead of passing in a Timespan parameter to represent the duration, we could pass in a DurationMeasurement type. We would then also need to extend the DurationMeasurement so that it contains a StartedAt or StartDateTime property which returns the start-timestamp of the measurement. Drawback of this solution is that people need to pass in a DurationMeasurement, and that might not always be available. (This can offcourse be solved by making sure that we can instantiate DurationMeasurement instances by passing in a DateTimeOffset and a Timespan when constructing it. (Either via a constructor or via a factory method).

  • We can pass - in the start-datetime as a property in the context parameter, but that is just implicit and not favorable.

@stijnmoreels
Copy link
Member

I think the draw-back of using DurationMeasurement is wrong here bc it already contains a StartTime/Duration so we can create overloads with that type and be done with it 🥳 .

@stijnmoreels stijnmoreels added this to the v2.5.0 milestone Mar 25, 2022
@stijnmoreels stijnmoreels added enhancement New feature or request application-insights All issues related to Azure Application Insights requests All issues related to requests labels Mar 25, 2022
@stijnmoreels stijnmoreels self-assigned this Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-insights All issues related to Azure Application Insights enhancement New feature or request requests All issues related to requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants