Skip to content

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

Merged
winder merged 1 commit intoalgorand:masterfrom
winder:will/docker-telemetry
Oct 4, 2023
Merged

docker: Do not disable telemetry if logging.config is present.#5769
winder merged 1 commit intoalgorand:masterfrom
winder:will/docker-telemetry

Conversation

@winder
Copy link
Copy Markdown
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
Copy Markdown

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 gmalouf and onetechnical October 3, 2023 17:09
@winder winder marked this pull request as ready for review October 3, 2023 17:09
Copy link
Copy Markdown
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