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

maint: Refactor Config #197

Merged
merged 28 commits into from
Sep 19, 2023
Merged

maint: Refactor Config #197

merged 28 commits into from
Sep 19, 2023

Conversation

MikeGoldsmith
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith commented Sep 18, 2023

Which problem is this PR solving?

Config options were scattered through the codebase. This PR centralises all configuration options into the Config package and updates usage to be consistent.

Short description of the changes

  • Move config vars into NewConfig
  • Remove flags.* usage
  • Add descriptions to Config struct fields
  • Move stats dataset name from assembler to config
  • Add stats dataset config option and allow to read from env var
  • Removes unused config options - max, statsevery, verbose, debug, quiet, fname and tstype
  • Add config validate func with Missing and Invalid error types
  • Add GetMaskedAPIKey func to config so we can safely log it
  • Add tests for key masking
  • Add env util for retrieving string and bool env vars with a default
  • Reorganise main.go, includes setup of logging, libhoney and k8s into their own funcs
  • Update README to include the env vars we currently support
  • Add test and docker-test makefile targets
  • Update main and pr github workflows to execute tests using new makefile targets

How to verify that this has the expected result

Config usage is much cleaner and easier to reason about.

Environment variables set in the k8s deployment are used when configuring the agent. For example, you can set HONEYCOMB_DATASET=some-other-place under env.

robbkidd and others added 5 commits September 12, 2023 16:29
…to team.queue-stats

# Conflicts:
#	assemblers/tcp_assembler.go
#	assemblers/tcp_stream.go
#	assemblers/tcp_stream_factory.go
#	config/config.go
@MikeGoldsmith MikeGoldsmith added the type: maintenance The necessary chores to keep the dust off. label Sep 18, 2023
@MikeGoldsmith MikeGoldsmith requested a review from a team September 18, 2023 12:11
@MikeGoldsmith MikeGoldsmith self-assigned this Sep 18, 2023
@MikeGoldsmith MikeGoldsmith changed the title feat: Refactor Config maint: Refactor Config Sep 18, 2023
main.go Outdated Show resolved Hide resolved
JamieDanielson and others added 2 commits September 18, 2023 18:05
Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
Copy link
Member

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

To get the env-var lookups working, I think we'll need to refactor further to remove the flag.* usage in favor of doing more in the NewConfig constructor or wrap the var x = flag.* statements in an init() {} function.

config/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

This is great, thanks Mike! I updated these environment variables in my .env file and updated my deployment.yaml and confirmed the updated values.

# .env
HONEYCOMB_API_KEY=<redacted>
HONEYCOMB_DATASET=jamie-agent
HONEYCOMB_STATS_DATASET=jamie-agent-stats
...
# deployment.yaml
            - name: HONEYCOMB_DATASET
              value: $HONEYCOMB_DATASET
            - name: HONEYCOMB_STATS_DATASET
              value: $HONEYCOMB_STATS_DATASET
2023-09-19 11:20:31 3:20PM INF Starting Honeycomb Network Agent agent_version=0.0.16-alpha api_key=******************1234 dataset=jamie-agent endpoint=https://api.honeycomb.io stats_dataset=jamie-agent-stats

JamieDanielson and others added 4 commits September 19, 2023 13:08
otherwise it will work off a default config and not accept
the incoming configured debug address

Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
@JamieDanielson
Copy link
Contributor

Updated the debug service start in main to use the computed config object , realized that was missing in testing with my environment variable testing. Also added a few clarifications and changes to the deployment yamls! Think this should be good to go now!

@robbkidd robbkidd merged commit ef65ddc into main Sep 19, 2023
3 checks passed
@robbkidd robbkidd deleted the team.queue-stats branch September 19, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: maintenance The necessary chores to keep the dust off.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants