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

add custom tags to internal metrics #231

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

shaan420
Copy link
Collaborator

@shaan420 shaan420 commented Oct 3, 2023

No description provided.

@shaan420 shaan420 requested a review from prateek October 3, 2023 20:25

// CommonTagsInternal are tags that should be added to all internal metrics
// emitted by the reporter.
CommonTagsInternal map[string]string `yaml:"commonTagsInternal"`
Copy link
Collaborator

@prateek prateek Oct 3, 2023

Choose a reason for hiding this comment

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

can you call this InternalTags or something (i just want the prefix to start with Internal to make it clear it's for internal usage)

m3/reporter.go Outdated
@@ -288,6 +289,11 @@ func NewReporter(opts Options) (Reporter, error) {
internalTags := map[string]string{
"version": tally.Version,
}

for k, v := range opts.CommonTagsInternal {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you ensure we don't set host/instance here too please (i think we might be inheriting those from common tags)

Copy link
Collaborator

@prateek prateek left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #231 (0985f9e) into master (be77342) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 0985f9e differs from pull request most recent head a5fc9af. Consider uploading reports for the commit a5fc9af to get more accurate results

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   47.82%   47.85%   +0.03%     
==========================================
  Files          64       64              
  Lines        5989     5993       +4     
==========================================
+ Hits         2864     2868       +4     
  Misses       2685     2685              
  Partials      440      440              
Files Coverage Δ
m3/config.go 100.00% <100.00%> (ø)
m3/reporter.go 92.91% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shaan420 shaan420 force-pushed the uber-go/tally/230/snair/custom-tags-internal branch from 0985f9e to a5fc9af Compare October 4, 2023 16:28
@shaan420 shaan420 merged commit acc0d3c into master Oct 4, 2023
6 checks passed
@shaan420 shaan420 deleted the uber-go/tally/230/snair/custom-tags-internal branch October 4, 2023 16:30
shaan420 added a commit that referenced this pull request Oct 6, 2023
* add custom tags to internal metrics

* make unit test stricter, update version
shaan420 added a commit that referenced this pull request Oct 6, 2023
* Set default reporting interval (#226)

* Enforce minimum reporting interval

* Add new RootScope constructor with default interval

* Test with go >= 1.18 (#228)

* add custom tags to internal metrics (#231)

* add custom tags to internal metrics

* make unit test stricter, update version

* update version to v3.5.6

---------

Co-authored-by: Vytenis Darulis <vytenis.darulis@chronosphere.io>
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.

2 participants