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

fix: Optimize export metric behavior #3047

Merged
merged 4 commits into from Jan 4, 2024

Conversation

Taylor-lagrange
Copy link
Contributor

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Optimize export metric behavior in pr #2928

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Avoid network when standalone and frontend are feeding themselves by ExportMetricHandler
  • Default export metric schema be set to information_schema after solve Duplicate information_schema #2976

About discuss on #2972

Because GreptimeDBTelemetryTask in metasrv is a bit complicate, so we’ll discuss it later

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.

Refer to a related PR or issue link (optional)

Closes #2975

@github-actions github-actions bot added Doc not needed This PR do not impact docs Size: M labels Dec 29, 2023
Copy link

codecov bot commented Dec 29, 2023

Codecov Report

Attention: 130 lines in your changes are missing coverage. Please review.

Comparison is base (1d80a0f) 85.76% compared to head (6f811c5) 85.18%.
Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3047      +/-   ##
==========================================
- Coverage   85.76%   85.18%   -0.58%     
==========================================
  Files         791      805      +14     
  Lines      127985   131093    +3108     
==========================================
+ Hits       109760   111674    +1914     
- Misses      18225    19419    +1194     

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

Almostly LGTM

src/servers/src/export_metrics.rs Outdated Show resolved Hide resolved
@killme2008
Copy link
Contributor

@v0y4g3r @fengjiachun PTAL

@zhongzc
Copy link
Contributor

zhongzc commented Jan 3, 2024

I wholeheartedly agree with the configuration mentioned by @killme2008. Frankly speaking, it would be hard for me to hit the Approve button if we didn't follow through with this approach. 🥲

Here are my opinions:
#2928 (comment)
#2928 (comment)

@Taylor-lagrange
Copy link
Contributor Author

In my original idea, I did retain the design of using the entire url as a parameter, for greater compatibility. In the subsequent review, the parameters are split to endpoint and db according to the advice. So we can reach an agreement to merge endpoint and db into one parameter (e.g. http://127.0.0.1/v1/prometheus/write?db=information_schema), which will have wider compatibility?

But as for distinguishing the two import methods (i.e. remote_write for url and self_import for internal), I personally think it may increase users' understanding. Because only frontend and standalone can use self_import in certain case, others have to rely on url, because there is no good way to know the frontend address in the cluster.

How about yours opinion? @killme2008 @zhongzc

@killme2008
Copy link
Contributor

In my original idea, I did retain the design of using the entire url as a parameter, for greater compatibility. In the subsequent review, the parameters are split to endpoint and db according to the advice. So we can reach an agreement to merge endpoint and db into one parameter (e.g. http://127.0.0.1/v1/prometheus/write?db=information_schema), which will have wider compatibility?

But as for distinguishing the two import methods (i.e. remote_write for url and self_import for internal), I personally think it may increase users' understanding. Because only frontend and standalone can use self_import in certain case, others have to rely on url, because there is no good way to know the frontend address in the cluster.

How about yours opinion? @killme2008 @zhongzc

Yes, totally agree with the URL as configured parameter instead of endpoint and db.

Considering that standalone would be the most installed version, i think it's valuable for user to useself_import, and we can make it default for standalone.

Copy link
Contributor

@zhongzc zhongzc left a comment

Choose a reason for hiding this comment

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

LGTM

@fengjiachun fengjiachun added this pull request to the merge queue Jan 4, 2024
@killme2008 killme2008 added Doc update required This change requires document update on https://github.com/GreptimeTeam/docs and removed Doc not needed This PR do not impact docs labels Jan 4, 2024
@github-actions github-actions bot added Doc not needed This PR do not impact docs and removed Doc update required This change requires document update on https://github.com/GreptimeTeam/docs labels Jan 4, 2024
Merged via the queue into GreptimeTeam:main with commit 4d250ed Jan 4, 2024
31 checks passed
@Taylor-lagrange Taylor-lagrange deleted the fix-export-metric branch January 4, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc not needed This PR do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow-up tasks of system metric
4 participants