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

Fix plugin tracing again 😭 #2073

Merged

Conversation

carolynvs
Copy link
Member

@carolynvs carolynvs commented May 18, 2022

What does this change

When we fixed recursive plugin loading in #2071, we did so by disabling config loading in the plugin. This was almost right but not quite. While Porter does resolve the relevant config for the plugin and pass it on stdin, this does not provide some of the general configuration settings that can be used by the plugin such as logging and telemetry.

Specifically, our change caused a regression where the plugins didn't load the telemetry configuration and stopped sending trace data.

I have updated how configuration data is loaded for plugins:

  1. When we start a plugin binary we grab relevant configuration settings and pass them as environment variables to the plugin.
  2. When loading configuration for a plugin, we skip reading just the config file, but we still load configuration using environment values.
  3. Added a new telemetry setting that redirects traces from the standard open telemetry collector to write them to a file instead (in PORTER_HOME/traces). This helped me finally write a regression test for sending trace data so that I don't accidentally break this again.

While I was working on this I found that we aren't handling close errors when we close porter's log file and we were missing a race condition where the plugin closes the log file before porter is done with it. I've split up the log and trace files for the plugins and porter so that they write to CORRELLATIONID.json for porter and
CORRELLATIONID-PLUGINKEY.json for the plugins.

What issue does it fix

Restores plugin telemetry.

Closes #2057

Notes for the reviewer

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

@carolynvs carolynvs force-pushed the fix-plugin-tracing-again-help-me branch from 99e9dd4 to 0c44ef1 Compare May 18, 2022 19:39
@carolynvs carolynvs marked this pull request as ready for review May 18, 2022 19:59
When we fixed recursive plugin loading in getporter#2071, we did so by disabling
config loading in the plugin. This was _almost_ right but not quite.
While Porter does resolve the relevant config for the plugin and pass it
on stdin, this does not provide some of the general configuration
settings that can be used by the plugin such as logging and telemetry.

Specifically, our change caused a regression where the plugins didn't
load the telemetry configuration and stopped sending trace data.

I have updated how configuration data is loaded for plugins:

1. When we start a plugin binary we grab relevant configuration settings
   and pass them as environment variables to the plugin.
2. When loading configuration for a plugin, we skip reading just the
   config file, but we still load configuration using environment
   values.
3. Added a new telemetry setting that redirects traces from the standard
   open telemetry collector to write them to a file instead (in
   PORTER_HOME/traces). This helped me finally write a regression test
   for sending trace data so that I don't accidentally break this again.

While I was working on this I found that we aren't handling close errors
when we close porter's log file and we were missing a race condition
where the plugin closes the log file before porter is done with it. I've
split up the log and trace files for the plugins and porter so that they
write to CORRELLATIONID.json for porter and
CORRELLATIONID-PLUGINKEY.json for the plugins.

Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs carolynvs force-pushed the fix-plugin-tracing-again-help-me branch from 0c44ef1 to 191aa1c Compare May 18, 2022 20:11
@carolynvs carolynvs requested a review from VinozzZ May 18, 2022 20:24
Copy link
Member

@vdice vdice 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 the context and breakdown of changes. LGTM!

@carolynvs carolynvs merged commit 1327474 into getporter:release/v1 May 19, 2022
@carolynvs carolynvs deleted the fix-plugin-tracing-again-help-me branch May 19, 2022 14:42
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.

2 participants