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

[coordinator] Use tag options specified in config with M3Msg ingester #2212

Merged

Conversation

robskillington
Copy link
Collaborator

What this PR does / why we need it:

Previously the default tag options were being used for the M3Msg ingester with the coordinator, this uses the configured tag options for the coordinator with the M3Msg ingester.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

@codecov
Copy link

codecov bot commented Mar 15, 2020

Codecov Report

Merging #2212 into master will increase coverage by 0.1%.
The diff coverage is 73.6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2212     +/-   ##
========================================
+ Coverage    72.1%   72.3%   +0.1%     
========================================
  Files        1022    1022             
  Lines       89022   88809    -213     
========================================
- Hits        64217   64213      -4     
+ Misses      20520   20281    -239     
- Partials     4285    4315     +30     
Flag Coverage Δ
#aggregator 82.0% <ø> (-0.1%) ⬇️
#cluster 85.3% <ø> (-0.1%) ⬇️
#collector 82.8% <ø> (ø)
#dbnode 78.9% <ø> (-0.2%) ⬇️
#m3em 74.4% <ø> (ø)
#m3ninx 72.3% <ø> (-0.2%) ⬇️
#m3nsch 51.1% <ø> (ø)
#metrics 17.6% <ø> (ø)
#msg 74.8% <ø> (ø)
#query 68.9% <73.6%> (+1.1%) ⬆️
#x 83.2% <ø> (-0.3%) ⬇️

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 66f5bd7...59c87a5. Read the comment docs.

Comment on lines 532 to 534
if downsamplerReadyCh != nil {
downsamplerReadyCh <- struct{}{}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete this? should get done in the defer right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is in an else clause - defer is inside an inner function inside the other side of the else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I think)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, then won’t you miss a send when the newDownsamplerFn errors on this branch? Can we defer before this if else to avoid this? Could see this being a bit dangerous going forward

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll move it into newDownsamplerFn so it's only done in one place.

I can't defer from the top level function since some of these are async clients and are not ready by the end of the top level function creating these async clients.

For some further contextDownsamplerReadyCh is only used by tests to ensure failures to write a datapoint doesn't fail due to the async downsampler being not ready. The DownsamplerReadyCh should not be notified unless the downsampler is truly ready, if an error occurs then the channel should never receive an update.

@robskillington robskillington merged commit 5289a5f into master Mar 15, 2020
@robskillington robskillington deleted the r/use-configured-tag-options-for-m3msg-ingester branch March 15, 2020 22:04
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