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: http header with metrics #3536

Merged
merged 11 commits into from
Mar 18, 2024

Conversation

shuiyisong
Copy link
Contributor

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

This pr is a follow-up of #3466.
Now that we have OutputMeta, we can easily bring metrics from execution level up to the HTTP interface and put them into HTTP header.
This pr covers basically every HTTP entry with read or write request, returning a HTTP header indicating the cost of the request.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Mar 18, 2024
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 57.84314% with 86 lines in your changes are missing coverage. Please review.

Project coverage is 84.96%. Comparing base (393ea44) to head (0619a30).
Report is 1 commits behind head on main.

❗ Current head 0619a30 differs from pull request most recent head 13c1d71. Consider uploading reports for the commit 13c1d71 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3536      +/-   ##
==========================================
- Coverage   85.38%   84.96%   -0.43%     
==========================================
  Files         906      906              
  Lines      151023   151078      +55     
==========================================
- Hits       128952   128361     -591     
- Misses      22071    22717     +646     

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

src/servers/src/http/prometheus.rs Outdated Show resolved Hide resolved
src/operator/src/statement/copy_table_from.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@v0y4g3r v0y4g3r left a comment

Choose a reason for hiding this comment

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

LGTM

@v0y4g3r v0y4g3r added this pull request to the merge queue Mar 18, 2024
Merged via the queue into GreptimeTeam:main with commit 3acd5bf Mar 18, 2024
17 checks passed
@tisonkun
Copy link
Collaborator

This seems a refactor rather than a chore ... What a big change set 🤣

@sunng87
Copy link
Member

sunng87 commented Mar 18, 2024

This is actually a feature 😹

@tisonkun tisonkun changed the title chore: http header with metrics feat: http header with metrics Mar 18, 2024
@tisonkun
Copy link
Collaborator

Although we miss the chance to correct the commit message, let's change the PR title ...

@shuiyisong shuiyisong deleted the chore/output_with_cost branch March 19, 2024 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants