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

Replace opentelemetry by tikv/rust-prometheus #3869

Conversation

eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Mar 6, 2023

What problem does this PR solve?

Issue Number: close #3866

Problem Summary:

This PR want to drop opentelemetry and replace it by rust-prometheus on util/metrics

What is changed and how it works?

What's Changed:

Related changes

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • None

Release note

Title Only: Include only the PR title in the release note.

@eval-exec eval-exec changed the title Feature/drop opentelemetry use tikv prometheus Replace opentelemetry by tikv/rust-prometheus Mar 6, 2023
network/src/network.rs Outdated Show resolved Hide resolved
@eval-exec eval-exec marked this pull request as ready for review March 6, 2023 12:52
@eval-exec eval-exec requested a review from a team as a code owner March 6, 2023 12:52
@eval-exec eval-exec requested review from doitian and removed request for a team March 6, 2023 12:52
@eval-exec eval-exec marked this pull request as draft March 6, 2023 12:55
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch 2 times, most recently from 03670c6 to 9e280ac Compare March 6, 2023 13:59
@eval-exec eval-exec marked this pull request as ready for review March 6, 2023 13:59
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch 3 times, most recently from 740425d to 62ea525 Compare March 6, 2023 15:05
util/metrics/src/lib.rs Outdated Show resolved Hide resolved
"duration" => duration.as_secs().to_string(),
"reason" => reason.clone(),
);
ckb_metrics::CKB_NETWORK_BAN_PEER.inc();
Copy link
Member

Choose a reason for hiding this comment

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

We may add reason as a label, but let's defer it until we really need this.

Copy link
Collaborator Author

@eval-exec eval-exec Mar 7, 2023

Choose a reason for hiding this comment

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

I think we can expose reason to Prometheus, but it's preferable for reason to be an enumerated value known at compile time. This way, reason won't occupy too much memory space.

Copy link
Member

Choose a reason for hiding this comment

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

It's easy to break, for example, if someone formats a message and adds the peer id into it.

@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch from 62ea525 to a1154db Compare March 7, 2023 01:08
@eval-exec eval-exec self-assigned this Mar 7, 2023
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch from a1154db to a4ae236 Compare March 7, 2023 01:30
@eval-exec
Copy link
Collaborator Author

eval-exec commented Mar 7, 2023

In current code base, metrics are always collected, but these metrics won't be exposed unless we specific [metrics.exporter.prometheus] in ckb.toml config file.

We don't have a way to disable to collect metrics.

@doitian Do we need to add a [feature] enable_metrics for CKB to control whether to collect metrics on these points?

@doitian
Copy link
Member

doitian commented Mar 7, 2023

In current code base, metrics are always collected, but these metrics won't be exposed unless we specific [metrics.exporter.prometheus] in ckb.toml config file.

We don't have a way to disable to collect metrics.

@doitian Do we need to add a [feature] enable_metrics for CKB to control whether to collect metrics on these points?

We can use a macro wrapper to turn the code like

ckb_metrics::CKB_NETWORK_BAN_PEER.inc();

into

if (metrics_enabled!()) {
    ckb_metrics::CKB_NETWORK_BAN_PEER.inc();
}

@eval-exec
Copy link
Collaborator Author

We can use a macro wrapper to turn the code like

Agreed, I will go ahead and implement it.

@eval-exec eval-exec marked this pull request as draft March 7, 2023 02:10
@eval-exec eval-exec marked this pull request as ready for review March 7, 2023 02:42
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch from 63385ad to fc41b69 Compare March 7, 2023 02:56
@eval-exec eval-exec marked this pull request as draft March 7, 2023 06:04
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch 2 times, most recently from 7c7e6fa to a1a66c8 Compare March 7, 2023 06:44
@eval-exec
Copy link
Collaborator Author

In the current implementation, the values of "labels" exposed in ckb_message_bytes are difficult to understand as they are represented by numbers. like:

ckb_message_bytes{direction="out",item_id="6",protocol="101",status="101"} 62
ckb_message_bytes{direction="out",item_id="7",protocol="101",status="101"} 11390

After communicating with @gpBlockchain, I should update the "labels" field with meaningful values, like:

ckb_message_bytes{direction="out",item_id="GetBlockProposal",protocol="RelayV2",status="Ignored"} 62
ckb_message_bytes{direction="out",item_id="BlockProposal",protocol="RelayV2",status="Ignored"} 11390

Cc. @doitian

@eval-exec eval-exec requested a review from driftluo March 7, 2023 14:32
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch 3 times, most recently from ca51a1a to cfe9daf Compare March 10, 2023 02:03
@eval-exec eval-exec marked this pull request as ready for review March 10, 2023 02:03
@zhangsoledad zhangsoledad added this pull request to the merge queue Mar 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 11, 2023
@eval-exec eval-exec marked this pull request as draft March 11, 2023 15:14
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch from cfe9daf to 006a5b6 Compare March 16, 2023 01:46
@eval-exec eval-exec marked this pull request as ready for review March 16, 2023 02:06
@eval-exec eval-exec marked this pull request as draft March 16, 2023 02:50
@eval-exec
Copy link
Collaborator Author

eval-exec commented Mar 16, 2023

Considering use Histogram type to record ckb_message_bytes.
https://prometheus.io/docs/practices/histograms/#quantiles

@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch from 04a6fd9 to 2841856 Compare March 20, 2023 01:50
@eval-exec eval-exec marked this pull request as ready for review March 20, 2023 01:50
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch from 2841856 to 82b6e48 Compare March 20, 2023 01:53
@eval-exec eval-exec force-pushed the feature/drop_opentelemetry_use_tikv_prometheus branch from 82b6e48 to f03426f Compare March 20, 2023 03:27
@eval-exec eval-exec requested a review from zhangsoledad March 20, 2023 03:28
@zhangsoledad zhangsoledad added this pull request to the merge queue Mar 20, 2023
@zhangsoledad zhangsoledad merged commit 8f848b3 into nervosnetwork:develop Mar 20, 2023
@eval-exec eval-exec deleted the feature/drop_opentelemetry_use_tikv_prometheus branch March 20, 2023 08:31
@doitian doitian mentioned this pull request Apr 13, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

metrics!(gauge) creates histogram metric in prometheus
4 participants