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

NH-34752 Add config JSON file support #133

Merged
merged 5 commits into from
Apr 17, 2023

Conversation

tammy-baylis-swi
Copy link
Contributor

Implements the stub for update_with_cnf_file and adds a helper update_transaction_filters. Both read values from a customer file at SW_APM_CONFIG_FILE else the default ./solarwinds-apm-config.json, to copy what Java APM does. Values are stored in the SolarWindsApmConfig init'd by Configurator and passed to our custom Otel components at service startup.

The config file JSON keys are a bit different than Java. Example:

  • config file: triggerTrace
  • env var: SW_APM_TRIGGER_TRACE

All thoughts/questions welcome!

Next PRs:

  • Actually use transaction_filters at should_sample
  • infodev/customer docs update

@tammy-baylis-swi tammy-baylis-swi requested review from cheempz and a team April 15, 2023 00:10
@tammy-baylis-swi tammy-baylis-swi marked this pull request as ready for review April 15, 2023 00:14
val = cnf_dict.get("transaction").get("prependDomain")
if val is not None:
self._set_config_value("transaction.prepend_domain_name", val)
except AttributeError:

Check notice

Code scanning / CodeQL

Empty except

'except' clause does nothing but pass and there is no explanatory comment.
Copy link
Member

@raphael-theriault-swi raphael-theriault-swi left a comment

Choose a reason for hiding this comment

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

Looks good, just had a small question

# TODO after alpha: is_lambda
for key in available_cnf:
cnf_key_parts = key.split("_")
cnf = f"{cnf_key_parts[0]}{''.join(part.title() for part in cnf_key_parts[1:])}"

Choose a reason for hiding this comment

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

Just to make sure I understand this goes from snake_case to camelCase ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes 👍

Choose a reason for hiding this comment

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

Maybe worth extracting to a function in a future revision to make it more evident at first glance

@tammy-baylis-swi tammy-baylis-swi merged commit 92ef170 into main Apr 17, 2023
@tammy-baylis-swi tammy-baylis-swi deleted the NH-34752-add-cnf-file-reading branch April 17, 2023 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants