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

Make GraphQL Crystal RFC Compliant #233

Closed
wants to merge 1 commit into from

Conversation

tusharmath
Copy link

Origin servers MUST include a Date header field in all responses,
except in these cases:

As per the RFC — https://datatracker.ietf.org/doc/html/rfc2616#section-14.18

@jgillich
Copy link
Member

Perhaps this should be addressed in Kemal. I'm not going to start checking every server for HTTP compliance.

@tusharmath
Copy link
Author

@jgillich If you want to create benchmarks that can be trusted, I think it's important to make sure there is consistency between the benchmarking setups. You can learn from Techempower benchmarks for example, as they perform multiple checks to make sure no hacking of benchmarks happens.

@jgillich
Copy link
Member

I never intended to make something as comprehensive as Techempower. Besides, one could argue that adding this header is actually less realistic since most people won't be doing that in their real world apps.

@tusharmath
Copy link
Author

While I understand your perspective on not aiming for a comprehensive benchmark like Techempower, I'd like to point out a crucial aspect regarding the 'Date' header in HTTP responses. From my observations, it appears that most servers, except the one implemented in Crystal, are sending this header. This discrepancy can lead to an unfair advantage for Crystal-based servers for two reasons.

Firstly, omitting the 'Date' header means there's no need to allocate memory for each new response. This reduction in memory allocation can significantly impact server performance, especially under high load. Secondly, the absence of this header also results in sending less data over the network, which could inadvertently improve performance metrics.

In the spirit of fair and equitable benchmarking, and to uphold the principles of open source collaboration, I strongly recommend considering contributions that aim to level the playing field. Ensuring that all frameworks are subject to the same requirements and constraints is vital for the integrity of these benchmarks.

Additionally, I propose the use of k6 instead of wrk for benchmarking. k6 is a modern performance testing tool that allows for more sophisticated tests and validations. This switch could provide a more robust framework for assessing performance across different setups, further enhancing the fairness and accuracy of our benchmarks.

@jgillich
Copy link
Member

No disagreement there, this header should be added by Kemal or Crystal HTTP. But there's already a 'no undocumented optimizations' rule because I think it's best to stick with the defaults, and this is just the reverse of this case; I have removed optimizations from graphql-crystal under this rule too, this isn't about giving an unfair advantage.

This did actually use k6 in the past, but its overhead was massive, therefore it was replaced with bombardier (8e35eaf).

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.

None yet

2 participants