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

Handle invalid metric tags in agent #391

Closed
mariusoe opened this issue Aug 29, 2019 · 1 comment
Closed

Handle invalid metric tags in agent #391

mariusoe opened this issue Aug 29, 2019 · 1 comment
Labels
area/agent bug Something isn't working good first issue Good for newcomers

Comments

@mariusoe
Copy link
Member

Currently, if the agent tries to tag a metric with a tag value with is not considered as valid (see https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/tags/TagValue.java) it will throw an exception and the hook which is recording the metric will be deactivated.

This should be handled, e.g. the tag value could be set to <invalid>. By doing this, bad configurations are also be visible using the metrics.

@mariusoe mariusoe added bug Something isn't working good first issue Good for newcomers area/agent labels Aug 29, 2019
@arolfes
Copy link
Contributor

arolfes commented Apr 9, 2020

Hi @mariusoe ,
from my point of view there are 2 quick solutions:

inside rocks.inspectit.ocelot.core.tags.CommonTagsManager

a) catch exception and change value example

            TagValue value = null;
            try {
                value = TagValue.create(v);
            } catch (IllegalArgumentException e) {
                log.warn("illegal tag value {} converted to <invalid>", v);
                value = TagValue.create("<invalid>");
            }
            tagContextBuilder.putLocal(key, value);

b) copy TagValue.isValid(String) and check the result

            if (!isTagValueValid(v)) {
                log.warn("illegal tag value {} converted to <invalid>", v);
                v = "<invalid>";
            }
            
            tagContextBuilder.putLocal(key, TagValue.create(v));
.
.
.
    private static boolean isTagValueValid(String value) {
        return value.length() <= TagValue.MAX_LENGTH && StringUtils.isPrintableString(value);
    }

do you have another approach in mind?

i would prefer solution a) because there we are independent from "what is a valid TagValue Definition by OpenCensus" but I don't really like to catch the IllegalArgumentException ...

what do you think?

arolfes added a commit to arolfes/inspectit-ocelot that referenced this issue Apr 9, 2020
util method in CommonTagsManager to create TagValue
for all invalid TagValues is <invalid> generated
arolfes added a commit to arolfes/inspectit-ocelot that referenced this issue Apr 9, 2020
util method in CommonTagsManager to create TagValue
for all invalid TagValues is <invalid> generated
arolfes added a commit to arolfes/inspectit-ocelot that referenced this issue Apr 9, 2020
util method in CommonTagsManager to create TagValue
for all invalid TagValues is <invalid> generated
arolfes added a commit to arolfes/inspectit-ocelot that referenced this issue Apr 14, 2020
reworked review findings
util method is in separate util class
don't catch IllegalArgumentException -> implemented a isTagValueValid method
arolfes added a commit to arolfes/inspectit-ocelot that referenced this issue Apr 14, 2020
implement TagUtils for eum-server component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants