Skip to content
This repository has been archived by the owner on Nov 7, 2022. It is now read-only.

Add metric labels and trace attributes to stackdriver exporter #537

Closed
wants to merge 2 commits into from
Closed

Add metric labels and trace attributes to stackdriver exporter #537

wants to merge 2 commits into from

Conversation

bakins
Copy link

@bakins bakins commented May 1, 2019

Add ability to set trace attributes and metric labels as described in https://github.com/census-ecosystem/opencensus-go-exporter-stackdriver/blob/master/stackdriver.go#L210-L228

We set these when using Stackdriver exporter directly in our applications to set pod, namespace, etc inside Kubernetes.

Should also allow one to add labels to address #519

@bakins bakins requested review from pjanotti and a team as code owners May 1, 2019 15:31
Copy link

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Hi @bakins, thanks for submitting this PR.

We've been using general mechanisms like global-tags instead of exporter specific ones, do they not work in your scenario?

EnableTracing bool `mapstructure:"enable_tracing,omitempty"`
EnableMetrics bool `mapstructure:"enable_metrics,omitempty"`
MetricPrefix string `mapstructure:"metric_prefix,omitempty"`
TraceAttributes map[string]interface{} `mapstructure:"trace_attributes,omitempty"`
Copy link

@pjanotti pjanotti May 2, 2019

Choose a reason for hiding this comment

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

Any reason to not use global tags instead? In general we've been approaching this type of issue making it happen in the oc proto format so all exporters can generate the same data.

We still don't have the same for metric labels and prefix but both are applicable too.

Copy link
Author

Choose a reason for hiding this comment

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

That URL 404's

Copy link

Choose a reason for hiding this comment

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

ops, just fixed the link

Copy link
Author

Choose a reason for hiding this comment

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

If you have links for other docs, that'd be great. My searching came up empty. If I can use an existing feature, I'd rather do so. Thanks!

Copy link

Choose a reason for hiding this comment

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

Right now we just have global tags for tracing https://github.com/census-instrumentation/opencensus-service#-global-tags but we should implement the equivalent for metrics /cc @songy23

Copy link

Choose a reason for hiding this comment

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

btw, there is an issue regarding naming: on OC we should refer to these as attributes instead of tags but the docs were not updated yet.

Copy link

Choose a reason for hiding this comment

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

another related feature for attributes is in the pipelines #536

Choose a reason for hiding this comment

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

Hi @bakins, given that #536 was merged today, do you still need to add some functionality from this PR?

exporter/stackdriverexporter/stackdriverexporter_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #537 into master will increase coverage by 0.48%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
+ Coverage    59.1%   59.59%   +0.48%     
==========================================
  Files          69       69              
  Lines        4519     4524       +5     
==========================================
+ Hits         2671     2696      +25     
+ Misses       1682     1657      -25     
- Partials      166      171       +5
Impacted Files Coverage Δ
exporter/stackdriverexporter/stackdriver.go 48.07% <100%> (+48.07%) ⬆️
exporter/jaegerexporter/jaeger.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e57bda2...3c6d018. Read the comment docs.

@pjanotti
Copy link

Hi @bakins, given that #536 was merged 11 days ago, do you still need to add some functionality from this PR?

@pjanotti
Copy link

@bakins I'm closing this PR, let's know if you still want to move ahead with it.

@pjanotti pjanotti closed this Jun 12, 2019
@bakins
Copy link
Author

bakins commented Sep 18, 2019

I missed the notification for this. Sorry about that. #536 looks good for spans, I'd like something like that for metrics as well. I can take a look unless it is already in progress.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants