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 statsd metrics collection for metric names with ":" #8906

Conversation

odin-delrio
Copy link
Contributor

@odin-delrio odin-delrio commented Apr 16, 2017

Statsd server is ignoring malformed metrics. This change introduces a basic sanitizing to metric names for avoid losing those metrics.

Why is this needed at this level?

Because current statsd clients for JAVA are not dealing with this... I proposed the change to that clients but I did not get an answer (here and here)

Is Spring Boot producing metric names with ":"?

Yes, if you are using hystrix you will get metrics like this one:

 "gauge.servo.hystrix.hystrixcommand.https://mydomain.com.myfeignservice#getsomething(string).rollingcountshortcircuited": 0.0,

This kind of metrics are simply ignored by statsd servers.

Do you need a workaround?

For those that need this fixed right now, you can override the bean definition:

  @Bean
  @ExportMetricWriter
  @ConditionalOnMissingBean
  @ConditionalOnProperty(prefix = "spring.metrics.export.statsd", name = "host")
  public StatsdMetricWriter statsdMetricWriter(MetricExportProperties properties) {
    MetricExportProperties.Statsd statsdProperties = properties.getStatsd();
    return new StatsdMetricWriter(statsdProperties.getPrefix(), statsdProperties.getHost(), statsdProperties.getPort()) {
      @Override
      public void increment(Delta<?> delta) {
        super.increment(new Delta<Number>(sanitizeMetricName(delta.getName()), delta.getValue(), delta.getTimestamp()));
      }

      @Override
      public void set(Metric<?> metric) {
        super.set(new Metric<Number>(sanitizeMetricName(metric.getName()), metric.getValue(), metric.getTimestamp()));
      }

      private String sanitizeMetricName(String name) {
        return name.replace(":", "");
      }
    };
  }

Statsd server is ignoring malformed metrics. This change introduces
a basic sanitizing to metric names for avoid losing those metrics.
@odin-delrio odin-delrio changed the title Fix statsd metrics collection for names with ":" Fix statsd metrics collection for metric names with ":" Apr 16, 2017
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 16, 2017
@philwebb
Copy link
Member

/cc @dsyer @spencergibb

@philwebb
Copy link
Member

Looking at https://github.com/b/statsd_spec is seems that : is reserved as a separator. It would be nice if your other PR was merged and the Java client took care of this, but I can't see any harm in us doing it as well.

@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Apr 17, 2017
Copy link
Member

@dsyer dsyer 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 might have suggested an alternative separator ("-" instead of ":" maybe?).

@philwebb philwebb added priority: normal type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-attention An issue we'd like other members of the team to review labels Apr 17, 2017
@philwebb philwebb added this to the 1.4.6 milestone Apr 17, 2017
@snicoll snicoll self-assigned this Apr 18, 2017
snicoll pushed a commit that referenced this pull request Apr 18, 2017
Statsd server is ignoring malformed metrics. This change introduces
a basic sanitizing to metric names for avoid losing those metrics.

See gh-8906
snicoll added a commit that referenced this pull request Apr 18, 2017
* pr/8906:
  Polish "Fix statsd metrics collection for names with ":""
  Fix statsd metrics collection for names with ":"
@snicoll snicoll closed this in 9a5346f Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants