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

feat(crwa): OpenTelemetry #7455

Closed
wants to merge 9 commits into from
Closed

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented Jan 25, 2023

WIP: This experimental and exploratory might be dropped.

Problem
We are investigating changing up our telemetry setup.

Changes

  • Instruments the CRWA with tracing using OpenTelemetry.
    • The structure of CRWA has dictated the telemetry implementation a little. It tends to have an error and then immediately invoke process.exit(1) so not many opportunities to hook into process end.
  • Removed the dependencies on the internal and telemetry packages.

Outstanding

  1. Discussions
  2. Redacting sensitive information
  3. Getting a correct CRWA version to report back
  4. Needs to point to the collector
  5. Need to analyse the available analysis (internal)
  6. This is all inside the CRWA package but some of it like building the resource information might be best placed within an updated telemetry package to be dryer.
  7. Maybe more of the dependencies can be removed now?

Notes

  1. The first couple of commits contain my experimenting in the CLI but those will be reverted.

Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

A couple quick comments:


// Telemetry
if (
!process.argv.includes('--no-telemetry') && // Must include '--no-telemetry' exactly because we want to do this check before any yargs. TODO: Communicate this on cmd help
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a little too long to be on the same line as the code:

Suggested change
!process.argv.includes('--no-telemetry') && // Must include '--no-telemetry' exactly because we want to do this check before any yargs. TODO: Communicate this on cmd help
// Must include '--no-telemetry' exactly because we want to do this check before any yargs.
// TODO: Communicate this on cmd help
!process.argv.includes('--no-telemetry') &&


// Telemetry
if (
!process.argv.includes('--no-telemetry') && // Must include '--no-telemetry' exactly because we want to do this check before any yargs. TODO: Communicate this on cmd help
Copy link
Contributor

Choose a reason for hiding this comment

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

I had the same problem in the CLI's index.js file. I need the value of the cwd option before I invoke yargs. You should be able to use the same solution I did:

let { cwd } = Parser(hideBin(process.argv))

So in your case...

import { hideBin, Parser } from 'yargs/helpers'

// ...

const { telemetry } = Parser(hideBin(process.argv))

Then check if telemetry is false later, no need to deal with the "no-" prefix, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Excellent I had a suspicion that it might have been a similar issue you'd dealt. I'll try out your solution for sure.

Comment on lines -95 to -100
.option('telemetry', {
default: true,
type: 'boolean',
describe:
'Enables sending telemetry events for this create command and all Redwood CLI commands https://telemetry.redwoodjs.com',
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the suggestion in another comment, you can leave this here. Solves the documentation problem!

@jtoar
Copy link
Contributor

jtoar commented Jan 29, 2023

Removed the dependencies on the internal and telemetry packages.

Awesome!

Comment on lines 62 to 66
// TODO: Get a better way of recording what version of CRWA is running because this is just null...
[SemanticResourceAttributes.SERVICE_VERSION]:
info.npmPackages != null
? info.npmPackages['@redwoodjs/core']?.installed
: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason you can't just get it from this package's package.json? Like in create-redwood-app.js

import { name, version } from '../package'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd been using that technique in my earlier experimenting and it worked fine. I only replaced it with a to-do because vscode was yelling at me. I'll happily add it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha—yelling at you with red squiggles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah but it's fine now. I must have been importing it with the extension or something before.

@jtoar
Copy link
Contributor

jtoar commented Jan 29, 2023

@Josh-Walker-GM could you provide instructions on how you're testing this locally? I'd like to try it out if possible

@jtoar jtoar added the release:feature This PR introduces a new feature label Jan 29, 2023
@Josh-Walker-GM
Copy link
Collaborator Author

Closed as superseded by #8043

@Josh-Walker-GM Josh-Walker-GM deleted the jgmw-crwa-opentelemetry branch April 20, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants