Skip to content

config: add proxy version to bootstrap#8386

Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/proxy_version
Sep 27, 2019
Merged

config: add proxy version to bootstrap#8386
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/proxy_version

Conversation

@ramaraochavali
Copy link
Copy Markdown
Contributor

This PR adds a proxy version to bootstrap which will be used to set server.version metric.
Risk Level: Low, optional config
Testing: Added tests
Docs Changes: Updated
Release Notes: Updated

Fixes #8318

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8386 was opened by ramaraochavali.

see: more, trace.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@mattklein123 @htuch PTAL

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks @ramaraochavali at a high level this looks good but I have one API question for @htuch

// Optional proxy version which will be used to set the value of :ref:`server.version statistic
// <server_statistics>` if specified. Envoy will not process this value, it will be sent as is to
// :ref:<stats sinks <envoy_api_msg_config.metrics.v2.StatsSink>.
google.protobuf.UInt64Value proxy_version = 19;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@htuch thoughts on whether this should go into the Node message?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. The Node message will eventually have a semantic version, but not one specified (it will come from the API, not Envoy build). There will also be a user agent string, which probably will include a git SHA in the case of Envoy.

I view the version here to be purely a stats convenience and not useful for anything in xDS. I think if we rename this to stats_server_version_override then this sorts this problem out.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo the naming comment thread, nice work!

// Optional proxy version which will be used to set the value of :ref:`server.version statistic
// <server_statistics>` if specified. Envoy will not process this value, it will be sent as is to
// :ref:<stats sinks <envoy_api_msg_config.metrics.v2.StatsSink>.
google.protobuf.UInt64Value proxy_version = 19;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. The Node message will eventually have a semantic version, but not one specified (it will come from the API, not Envoy build). There will also be a user agent string, which probably will include a git SHA in the case of Envoy.

I view the version here to be purely a stats convenience and not useful for anything in xDS. I think if we rename this to stats_server_version_override then this sorts this problem out.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@htuch @mattklein123 changed as suggested. PTAL

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 self-assigned this Sep 27, 2019
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 8387d60 into envoyproxy:master Sep 27, 2019
@ramaraochavali ramaraochavali deleted the fix/proxy_version branch September 28, 2019 04:12
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
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.

custom proxy version as stat

3 participants