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

Change dbmodel.TagFilter to take span as argument #772

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

yurishkuro
Copy link
Member

Because sometimes the filtering decision depends not only on the tags, but on the span itself, e.g. debug bit, or the service name.

Signed-off-by: Yuri Shkuro ys@uber.com

Signed-off-by: Yuri Shkuro <ys@uber.com>
Copy link
Contributor

@vprithvi vprithvi left a comment

Choose a reason for hiding this comment

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

I'm not a fan of this approach because the interface is essentially accepting the same information twice.

Perhaps having a variant that takes in only the full span instead of the both the span and tags could work out cleaner

@black-adder
Copy link
Contributor

Chaining becomes a bit funky but I think we can get it to work

@black-adder
Copy link
Contributor

Alternative, keep the interface as is but have db.Model span implement it

@yurishkuro
Copy link
Member Author

Alternative, keep the interface as is but have db.Model span implement it

I need to control the implementation of the filter. I cannot change the implementation of the span when doing the filtering. So how would that help? Also, filters are static.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d6dc147 on storage-tag-filter-takes-span into 59cc364 on master.

@yurishkuro yurishkuro merged commit 0522e48 into master Apr 12, 2018
@ghost ghost removed the review label Apr 12, 2018
@yurishkuro yurishkuro deleted the storage-tag-filter-takes-span branch April 12, 2018 21:15
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.

4 participants