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

envoy: c911818721586 #2107

Merged
merged 2 commits into from
Mar 17, 2022
Merged

envoy: c911818721586 #2107

merged 2 commits into from
Mar 17, 2022

Conversation

junr03
Copy link
Member

@junr03 junr03 commented Mar 16, 2022

Includes envoyproxy/envoy#19758 so we now need to set the config option.

Will wait until @snowp resolves the upstream break in #2105 to land this

Jose Nino added 2 commits March 16, 2022 11:58
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
@@ -326,6 +326,7 @@ R"(
dns_cache_config: *dns_cache_config
transport_socket: *base_tls_socket
upstream_connection_options: &upstream_opts
set_local_interface_name_on_upstream_connections: true
Copy link
Contributor

Choose a reason for hiding this comment

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

How much of a perf impact does this have for EM use cases compared to turning it off? Would it be worth offering as a config option or turning off when log level is set to some values?

Copy link
Member Author

Choose a reason for hiding this comment

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

#2111 good point. Alternatively there is further work in upstream envoy that can make this functionality more performant.

For this PR though I'm biased to leave it on by default as that has been the case for all experiments we've run in the last quarter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'm ok with that too, but was curious if you had measured the impact on us.

@junr03 junr03 merged commit cb9fab8 into main Mar 17, 2022
@junr03 junr03 deleted the c911818721586 branch March 17, 2022 18:18
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.

2 participants