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

Only log error (don't also index it) if xpack is enabled. #12265

Merged
merged 2 commits into from
May 28, 2019

Conversation

ycombinator
Copy link
Contributor

When xpack.enabled: true is set on a stack module, the expectation is that the user won't see any metricbeat-* indices. Instead users expect to see .monitoring-* indices.

However, metricbeat indexes errors into metricbeat-* indices. So in an error situation when xpack.enabled: true is set, we don't want to index errors but just log them. That's what this PR fixes for the kibana/stats metricset.

@ycombinator ycombinator requested review from cachedout and ruflin May 24, 2019 05:06
@ycombinator ycombinator requested a review from a team as a code owner May 24, 2019 05:06
@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring

@ycombinator ycombinator force-pushed the mb-kb-only-log-error-xp branch from 4fa3781 to f2ee95a Compare May 24, 2019 14:18
Indexing it would index it into metricbeat-* indices instead of .monitoring-* indices, which are currently not equipped to handle error documents.
@ycombinator ycombinator force-pushed the mb-kb-only-log-error-xp branch from f2ee95a to 1e3ac44 Compare May 25, 2019 00:34
@ycombinator ycombinator merged commit 0900e01 into elastic:master May 28, 2019
@ycombinator ycombinator deleted the mb-kb-only-log-error-xp branch May 28, 2019 12:11
@ycombinator
Copy link
Contributor Author

ycombinator commented May 28, 2019

@ruflin Since this PR is a bugfix PR, I'm thinking about backporting it to 7.1 and 6.8 branches as well. However, for the backports to be successful, I also need to backport #11763 and, before that, #10727. Any concerns from your end about doing all these backports or should I go ahead?

@ruflin
Copy link
Contributor

ruflin commented May 29, 2019

If we backport to 7.1 / 6.8 I would hope we don't need to backport all the interface changes. I'm a bit worried about unexpected side effects.

Would it be possible to have the same behaviour in 6.8 without needing the new interface?

@cachedout
Copy link
Contributor

As an aside:

When xpack.enabled: true is set on a stack module, the expectation is that the user won't see any metricbeat-* indices. Instead users expect to see .monitoring-* indices.

Do we have this documented anywhere? I can't recall a place where we discuss the actual indexes where data lands. I looked briefly here: https://www.elastic.co/guide/en/elasticsearch/reference/7.1/configuring-metricbeat.html

I'm a little on the fence about whether or not we should document the relationship between the various metric shippers and the indexes which they write to, since this feels more like an implementation detail that a user should (hopefully?) be unconcerned with.

@ycombinator
Copy link
Contributor Author

If we backport to 7.1 / 6.8 I would hope we don't need to backport all the interface changes. I'm a bit worried about unexpected side effects.

Yes, this is my concern as well, which is why I pinged you first.

Would it be possible to have the same behaviour in 6.8 without needing the new interface?

I think this should be possible. I'll put up a non-backport PR for 7.1 and we can backport that one to 6.8.

@ycombinator
Copy link
Contributor Author

As an aside:
...

Yeah, I'm on the fence about this as well. It's definitely an implementation detail but I checked existing docs and we do mention .monitoring-* indices in a couple of places.

Plus I think it might save users a bit of surprise if we can tell them to expect .monitoring-* indices when xpack.enabled: true is set. @lcawl any thoughts on where and how best to document this?

@lcawl
Copy link
Contributor

lcawl commented May 29, 2019

When I was documenting the method to configure ESMS (elastic/stack-docs#318), @pickypg used the existence of certain indices to determine whether I had set things up properly.
I therefore drafted new "troubleshooting monitoring" information in that PR. If we need to go into greater detail there, that's fine by me.

@ycombinator ycombinator removed the needs_backport PR is waiting to be backported to other branches. label May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants