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

Management Server Identification #5770

Closed
ramaraochavali opened this issue Jan 30, 2019 · 8 comments
Closed

Management Server Identification #5770

ramaraochavali opened this issue Jan 30, 2019 · 8 comments
Assignees
Labels
design proposal Needs design doc/proposal before implementation enhancement Feature requests. Not bugs or questions. stale stalebot believes this issue/PR has not been touched recently

Comments

@ramaraochavali
Copy link
Contributor

Some times we would like to know to which management server instance, the running Envoy is connected to debug certain scenarios. Currently there is no provision to do that.

The proposal is to include a opaque identifier in DiscoveryResponse (similar to opaque identifier in Node of DiscoveryRequest) that management server can populate and send. On the Envoy side we can push a gauge stat indicating the management server identifier.

What do you think about this? Would you think it would be generally useful or leave it to the management server to figure that out by some logging etc.

@mattklein123 mattklein123 added the design proposal Needs design doc/proposal before implementation label Jan 30, 2019
@mattklein123
Copy link
Member

At the API level this SGTM, however I'm not sure we should output this as a numeric gauge given that it's likely to be a string. Can this be output as some sort of admin endpoint? Potentially as part of config_dump?

@htuch
Copy link
Member

htuch commented Jan 31, 2019

Yeah, I think this should be a Struct or something, essentially a JSON object we can dump out on admin. Also related somewhat to #5405 (but in the other direction..).

@ramaraochavali
Copy link
Contributor Author

ramaraochavali commented Jan 31, 2019

I thought about being able to dump on admin endpoint however in we (typically many others as well) do not have access to the admin end point with out going to the box in production. So was thinking it would be easier if we output via stats but seems making the identifier to completely numeric is restrictive in nature.
However can we introduce a message that has the numeric component as well (similar to instanceid or some thing) and we output that as stat and the entire structure in config_dump?

or any other ideas?

@mattklein123
Copy link
Member

I'm really not in favor of a numeric stat output here. I just don't think it makes sense.

One option here is potentially to create a new textual stat type, since I think some stat backends do support text? @fredlas is already thinking about a new gauge type that doesn't allow summing across hot restarts so maybe sync up with him?

@ramaraochavali
Copy link
Contributor Author

@mattklein123 Agree with using(rather misusing) numeric stat for this. I guess it is out of desperation rather than a perfect fit. If new textual stat is available that would be awesome.

Let us add api and output it to config_dump in v1 for management server identification. Once that stat type is available we could just output it to stat as well (which would be very easy thing to do).

If this plan makes sense, please assign this to me.

@mattklein123
Copy link
Member

Yes let's start with the API change and go from there.

@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Feb 1, 2019
htuch pushed a commit that referenced this issue Feb 7, 2019
This PR adds API support required for #5770 . Once #5844 lands, I will output it using the text stat.

Risk Level: Low
Testing: N/A

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@stale
Copy link

stale bot commented Mar 3, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 3, 2019
fredlas pushed a commit to fredlas/envoy that referenced this issue Mar 5, 2019
This PR adds API support required for envoyproxy#5770 . Once envoyproxy#5844 lands, I will output it using the text stat.

Risk Level: Low
Testing: N/A

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@stale
Copy link

stale bot commented Mar 10, 2019

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design proposal Needs design doc/proposal before implementation enhancement Feature requests. Not bugs or questions. stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
3 participants