-
Notifications
You must be signed in to change notification settings - Fork 199
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 metric filtering capability #1728
Conversation
validateSpanProcessorIncludeExclude(includeExclude); | ||
break; | ||
case METRIC_FILTER: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename it to METRIC?
} | ||
} | ||
} | ||
|
||
public static class ProcessorIncludeExclude { | ||
public MatchType matchType; | ||
public List<String> spanNames = new ArrayList<>(); | ||
public List<String> metricNames = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not renaming to METRIC, can you rename this to metricFilterNames for consistency.
if (attributes.isEmpty()) { | ||
throw new FriendlyException("A log processor configuration has an " + includeExclude + " section with no \"attributes\".", | ||
"Please provide \"attributes\" under the " + includeExclude + " section of the log processor configuration. " + | ||
"Learn more about log processors here: https://go.microsoft.com/fwlink/?linkid=2151557"); | ||
} | ||
|
||
validateSectionIsEmpty(spanNames, ProcessorType.LOG, includeExclude, "spanNames"); | ||
validateSectionIsEmpty(metricNames, ProcessorType.LOG, includeExclude, "metricNames"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metricFilterNames? or rename the processor type to METRIC?
"type": "metric-filter", | ||
"exclude": { | ||
"matchType": "strict", | ||
"metricNames": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see why metricNames makes sense now. please ignore renaming comments.
// Moshi JSON builder do not allow case insensitive mapping | ||
strict, regexp | ||
@Json(name = "strict") | ||
STRICT { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear to me why you need to define MatchType in both places.. one here and calling MetricFilter's constants.. why not just keep one enum here?
} | ||
} | ||
|
||
public enum MatchType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why not just define this in configuration.java?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because core doesn't depend on agent (where configuration is)
I left a couple of comments in Configuration about this:
// TODO (trask) this could be revisited in the future once core goes away
// need a different MetricFilter in this case because it lives in core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i saw that.. didn't quite get it.. ok, now it makes sense.
* Add metric filtering capability * Change enums to uppercase * Spotbugs
Resolves #1665