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

docker: Do not disable telemetry if logging.config is present. #5769

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

winder
Copy link
Contributor

@winder winder commented Oct 3, 2023

Summary

The docker image proactively disables telemetry if TELEMETRY_NAME is not set. Presumably this was to allow turning telemetry on and off for an existing container. This has a side effect of disabling telemetry enabled by a logging.config override.

We have two options:

  1. this is desirable behavior even when providing logging.config. No change is necessary.
  2. (this PR) If logging.config is provided and no TELEMETRY_NAME is provided, skip disabling telemetry.

Test Plan

Manually tested the bash script and ran it through shellcheck.

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #5769 (abd3981) into master (79fb8fe) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5769      +/-   ##
==========================================
- Coverage   55.50%   55.44%   -0.06%     
==========================================
  Files         473      473              
  Lines       66684    66684              
==========================================
- Hits        37010    36975      -35     
- Misses      27156    27187      +31     
- Partials     2518     2522       +4     

see 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@winder winder added the Bug-Fix label Oct 3, 2023
@winder winder requested review from onetechnical and gmalouf October 3, 2023 17:09
@winder winder marked this pull request as ready for review October 3, 2023 17:09
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

I think your suggestion is the right approach - we may need to communicate this a bit though hard to tell to whom..

@winder winder merged commit 690b07e into algorand:master Oct 4, 2023
@winder winder deleted the will/docker-telemetry branch October 4, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants