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.

Sign up for GitHub

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

Jump to bottom

Dedup collector tags #2658

Merged
merged 3 commits into from
Dec 7, 2020
Merged

Dedup collector tags #2658

merged 3 commits into from
Dec 7, 2020

Conversation

Betula-L
Copy link
Contributor

@Betula-L Betula-L requested a review from a team as a code owner November 27, 2020 04:24
@Betula-L Betula-L requested a review from vprithvi November 27, 2020 04:24
@mergify mergify bot requested a review from jpkrohling November 27, 2020 04:24
@Betula-L Betula-L force-pushed the dedup-collector-tags branch 2 times, most recently from fc6efe9 to 2237539 Compare November 27, 2020 04:26
@Betula-L
dedup collector tags
f303fae 
Signed-off-by: luhualin <luhualin@bilibili.com>
@Betula-L Betula-L force-pushed the dedup-collector-tags branch from 2237539 to f303fae Compare November 27, 2020 04:33
@codecov
Copy link

codecov bot commented Nov 27, 2020 •
edited
Loading

Codecov Report

Merging #2658 (3fb5704) into master (24fb6bc) will increase coverage by 0.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2658      +/-   ##
==========================================
+ Coverage   95.10%   95.55%   +0.45%     
==========================================
  Files         213      214       +1     
  Lines        9491     9555      +64     
==========================================
+ Hits         9026     9130     +104     
+ Misses        388      346      -42     
- Partials       77       79       +2
Impacted Files Coverage Δ
cmd/collector/app/span_processor.go 100.00% <100.00%> (ø)
cmd/query/app/static_handler.go 83.03% <0.00%> (-1.79%) ⬇️
plugin/storage/es/factory.go 100.00% <0.00%> (ø)
plugin/storage/es/options.go 100.00% <0.00%> (ø)
plugin/storage/es/spanstore/reader.go 100.00% <0.00%> (ø)
plugin/storage/es/spanstore/writer.go 100.00% <0.00%> (ø)
pkg/httpmetrics/metrics.go 100.00% <0.00%> (ø)
plugin/storage/es/dependencystore/storage.go 91.11% <0.00%> (+0.20%) ⬆️
cmd/collector/app/collector.go 74.64% <0.00%> (+0.36%) ⬆️
cmd/collector/app/server/zipkin.go 76.92% <0.00%> (+76.92%) ⬆️
... and 1 more

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 24fb6bc...3fb5704. Read the comment docs.

@Betula-L Betula-L force-pushed the dedup-collector-tags branch from ef105a7 to f13c582 Compare November 27, 2020 05:53
jpkrohling
jpkrohling reviewed Nov 27, 2020
View reviewed changes
Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

cmd/collector/app/span_processor.go Show resolved Hide resolved
cmd/collector/app/span_processor_test.go Outdated Show resolved Hide resolved
@Betula-L Betula-L force-pushed the dedup-collector-tags branch from f13c582 to 4764f92 Compare November 30, 2020 04:07
jpkrohling
jpkrohling reviewed Nov 30, 2020
View reviewed changes
Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Looks good. I have a question about a test that was removed, but other than that, it's ready to be merged.

cmd/collector/app/span_processor.go Outdated Show resolved Hide resolved
cmd/collector/app/span_processor_test.go Outdated Show resolved Hide resolved
@Betula-L
revise debug log
72c1873 
Signed-off-by: luhualin <luhualin@bilibili.com>
@Betula-L Betula-L force-pushed the dedup-collector-tags branch from 4764f92 to 72c1873 Compare November 30, 2020 10:43
@Betula-L
Copy link
Contributor Author

@jpkrohling Is it ready to be merged now?

jpkrohling
jpkrohling previously approved these changes Nov 30, 2020
View reviewed changes
@jpkrohling
Copy link
Contributor

LGTM. I'll leave it open for another couple of hours, as @yurishkuro likes to review PRs during breakfast :-)

@Betula-L
skip add tags if empty
3fb5704 
Signed-off-by: luhualin <luhualin@bilibili.com>
@mergify mergify bot dismissed jpkrohling’s stale review December 7, 2020 02:58

Pull request has been modified.

@yurishkuro yurishkuro merged commit 0b43839 into jaegertracing:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@jpkrohling jpkrohling jpkrohling left review comments

@vprithvi vprithvi Awaiting requested review from vprithvi

Assignees

@jpkrohling jpkrohling

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow de-duplicating Span.Process, agent, and collector tags
3 participants
@Betula-L @jpkrohling @yurishkuro