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
@Exagone313
Copy link
Contributor Author

Exagone313 commented Oct 18, 2024

@TwiN I'm thinking about working on this PR again. But I'm not quite sure about what should be implemented.

Let alone the naming, it could be possible to add new placeholders such as DNS_RESPONSE_TIME (and others possible with httptracing - to be implemented also on other protocols), but I think that computing RESPONSE_TIME - DNS_RESPONSE_TIME in a new placeholder would not make sense.

What do you think about adding an element of language to be able to add operations? For example a function subtract:

subtract(RESPONSE_TIME, DNS_RESPONSE_TIME) < 2s

I'm not sure if it should support nested function calls, e.g. subtract(subtract(RESPONSE_TIME, DNS_RESPONSE_TIME), FIRST_BYTE_TIME), as it would increase the complexity of the condition parser. Maybe for a future version.

Adding a function add would make sense along with subtract.

Another idea (which needs new parsing), an exp (short for expression) function, e.g. exp(RESPONSE_TIME - DNS_RESPONSE_TIME - FIRST_BYTE_TIME).

@TwiN
Copy link
Owner

TwiN commented Dec 28, 2024

I really like your idea of adding the add and subtract functions, if you're up for the challenge. exp, however, may be a bit too difficult to implement. That said, conditions is something I'd like to improve as well, since it is currently somewhat lacking. For instance, supporting [STATUS] == 200 || [STATUS] == 429 would be nice, as opposed to how I currently implemented ([STATUS] == any(200, 429)), but it would require a significant amount of work, which is why perhaps going the add and substract route may be easier.

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