Skip to content

xds: make log levels consistent#8320

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/inconsistent_log_levels
Sep 26, 2019
Merged

xds: make log levels consistent#8320
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/inconsistent_log_levels

Conversation

@ramaraochavali
Copy link
Copy Markdown
Contributor

Log levels between cds and lds are inconsistent in the way resource additions are logged. cds logs them at debug and lds logs them at info. This PR makes it consistent. If it is intentionally done like that, I am happy to close this PR

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
} else {
auto& cluster_entry = warming_clusters_.at(cluster_name);
ENVOY_LOG(info, "add/update cluster {} starting warming", cluster_name);
ENVOY_LOG(debug, "add/update cluster {} starting warming", cluster_name);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the above if block, these are logged at debug level, so making it consistent with that.

for (const auto& removed_listener : removed_resources) {
if (listener_manager_.removeListener(removed_listener)) {
ENVOY_LOG(info, "lds: remove listener '{}'", removed_listener);
ENVOY_LOG(debug, "lds: remove listener '{}'", removed_listener);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In cds, these are logged at debug level - so making it consistent with that.

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 for this fix. It makes sense to me to make this consistent, though I'm not sure if these messages should be info or debug. I don't have a strong opinion. @lambdai any thought here?

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Sep 24, 2019

Honestly I didn't pay enough attention to the log prior to this PR.
Personally I think added/removed/warmed are readable for both envoy developers and sysadmin. I prefer leaving these to info. So is the cds.

WDYT?

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@lambdai Sure. I have no strong opinion on whether it should be info or debug - but was looking for consistency. I can change them to info.

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

Ok. I changed the cds logs to be info so that they are consistent with lds. I have retained the cluster_manager_impl logs at debug as cds info level logs would be sufficient for cluster additions/removals.

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Sep 25, 2019

/lgtm
@ramaraochavali Thank you so much! I believe we would benefit from the change when I debug in the short future!

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Sep 25, 2019

/retest envoy-linux

@repokitteh-read-only
Copy link
Copy Markdown

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8320 (comment) was created by @lambdai.

see: more, trace.

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Commenter does not have sufficient privileges for PR 8320 in repo envoyproxy/envoy

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@mattklein123 This is stuck with azure pipeline, can you PTAL if this makes sense?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
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.

LGTM, thanks!

@mattklein123 mattklein123 merged commit 0558038 into envoyproxy:master Sep 26, 2019
@ramaraochavali ramaraochavali deleted the fix/inconsistent_log_levels branch September 27, 2019 03:59
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.

3 participants