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

services/horizon/internal/txsub: Add metrics to track which types of transaction Horizon is receiving #2553

Merged
merged 3 commits into from
May 5, 2020

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented May 5, 2020

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Add metrics to track which types of transaction (v0, v1, or fee bump) Horizon is receiving.

Why

Prior to protocol 13 there was only kind of transaction you could submit to Horizon. With these metrics we can monitor how many people actually use fee bump transactions and how many people are still submitting v0 transactions.

Note that we could also add hooks to ingestion which monitor the total number of fee bump transactions in a ledger. However, we will not be able to monitor the number of v0 transactions which were included in a ledger from the ingestion code because, once protocol 13 is enabled, Stellar Core will rewrite v0 transactions as v1 transactions.

Known limitations

We shouldn't have a separate metrics for each transaction type.

When you have multiple metrics that you want to add/average/sum, they should usually be one metric with labels rather than multiple metrics.

For example, rather than http_responses_500_total and http_responses_403_total, create a single metric called http_responses_total with a code label for the HTTP response code. You can then process the entire metric as one in rules and graphs.

From https://prometheus.io/docs/practices/instrumentation/#use-labels

This issue also applies to many of the existing horizon metrics such as txsub.succeeded and txsub.failed.

The go-metrics library does not support creating metrics with tags or labels, see rcrowley/go-metrics#135 . We should consider using the prometheus client directly for metrics https://github.com/go-kit/kit/blob/master/metrics/prometheus/prometheus.go . Alternatively we could use a library like https://github.com/armon/go-metrics which does support creating metrics with labels.

@cla-bot cla-bot bot added the cla: yes label May 5, 2020
@tamirms tamirms requested a review from a team May 5, 2020 12:11
Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

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

LGTM! I wonder if it would be also useful to add this value in logging and create aggregations in Kibana. One advantage is that we can track down specific request that sent a given envelope type (not sure if it's needed but maybe we can find who is still using old SDK using Referer header - we can discuss later today).

@tamirms tamirms merged commit 7c4596d into stellar:master May 5, 2020
@tamirms tamirms deleted the tx-metrics branch May 5, 2020 16:22
@bartekn
Copy link
Contributor

bartekn commented May 5, 2020

Thanks for 7471397!

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.

3 participants