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(core): do not include DNS query time in HTTP response time #526

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Exagone313
Copy link
Contributor

Fix #49

Summary

See #49

  • Fixing response time for other request types needs to be added in a newer issue
  • I don't know how to add a test for this, it would need some kind of mock for getting the time to be able to override it in the test.

Checklist

  • Tested and/or added tests to validate that the changes work as intended, if applicable.
  • Updated documentation in README.md, if applicable.

@TwiN
Copy link
Owner

TwiN commented Jul 20, 2023

I know the issue's been open for a while, but I'm perplexed about whether this is the right call.
In a real-life production scenario, a DNS call does occur, so is excluding the DNS call from Gatus really the right move? 🤔

@Exagone313
Copy link
Contributor Author

I can probably make this an optional feature (disabled by default) for each HTTP endpoint. But then maybe it makes more sense to add the feature for each kind of request.

@Exagone313
Copy link
Contributor Author

Exagone313 commented Sep 3, 2023

@TwiN What do you think about creating new placeholders specifically for the values for DNS query time and protocol response time, and leaving the [RESPONSE_TIME] placeholder untouched?

My patch has to be improved in any case because it is not clear if the DNSDone event can be triggered multiple times (is it asynchronous? what happens when there are multiple records for a name?)

@Exagone313 Exagone313 marked this pull request as draft September 3, 2023 14:04
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.

Response time includes DNS request time
2 participants