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

Update jolokia.go #1639

Closed
wants to merge 1 commit into from
Closed

Conversation

InfiniteCode
Copy link

Required for all PRs:

  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

Some of the values returned from jolokia have three levels of nesting. This fixes the issue of maps being returned instead of values.

Some of the values returned from jolokia have three levels of nesting. This fixes the issue of maps being returned instead of values.
case map[string]interface{}:
for k3, v3 := range t3 {
fields[measurement+"_"+k+"_"+k2+"_"+k3] = v3
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Holy nesting batman!

I think a programmatic solution would be better, not an explicit 3-key map.

For example:

func extractValues(k string, v interface{}, fields map[string]interface{}) {
    if v, ok := v.(map[string]interface{}); ok {
        for k2, v2 := range v {
            extractValues(k+"_"+k2, v2, fields)
        }
    } else {
        fields[k] = v
    }
}

func Gather() {
    ...
    if values, ok := out["value"]; ok {
        extractValues(measurement, values, fields)
    }
    ...
}

Copy link
Author

Choose a reason for hiding this comment

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

Totally agree. I don't think there ever will be more than 3 layers of nesting, but in a long term I think a recursive call is definitely a better solution.

We also need to be careful with recursion in general, in case there is some kind of issues might happen and recursion tree becomes too long. Probably limiting depth should be included.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well we're unmarshalling json, so cyclical structures aren't possible, so there's no risk of infinite recursion. You'd have to get thousands of levels deep before it would become an issue.
I guess what I'm saying is that I don't see the need for a depth limit. But the code complexity to add it is pretty minor, so I'm not really opposed to it either.

@sparrc
Copy link
Contributor

sparrc commented Aug 31, 2016

I believe this will fix #1693 once merged.

@InfiniteCode do you think you could refactor the code to make a recursive call? I agree with @phemmer that there isn't much risk of infinite recursion when we're only talking about marshaling JSON.

@ScubaDrew
Copy link

Can we merge this as-is to get this working until infiniteCode has time to refactor? It is 8 lines of code that will cover the vast majority of jolokia configurations.

@sparrc
Copy link
Contributor

sparrc commented Sep 15, 2016

sorry @ScubaDrew, but in my experience once it's in it will never go away, in addition to being harder to understand and harder to refactor in the future.

If someone else can take on properly refactoring and testing the code I'd appreciate it a lot. In the meantime I'm not going to merge in a hack.

@ScubaDrew
Copy link

@sparrc -- Understandable and thanks for the consideration. I have an enviornment running 1.0.0 which is suffering from this issue and thus is failing to collect many of the jolokia metrics.

@sparrc
Copy link
Contributor

sparrc commented Nov 3, 2016

closing for lack of updates on requested changes, please feel free to resubmit a PR if you can address the requested changes

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