Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

feat(one-app-runner): add option to load jaeger all-in-one #635

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

10xLaCroixDrinker
Copy link
Member

@10xLaCroixDrinker 10xLaCroixDrinker commented Mar 30, 2024

Description

Introduces new option to one-app-runner --include-jaeger

I also did a significant refactor/cleanup

  • Corrections to one-app-runner README
  • packages/one-app-runner/__tests__/bin/one-app-runner.spec.js
    • This included a lot of large, unuseful snapshots. I inlined the small, useful parts or just about all of them
    • use jest.isolateModules instead of jest.resetModules
  • packages/one-app-runner/__tests__/src/startApp.spec.js
    • update mocks setup
    • fixed conditional expect
    • largely functionally the same apart from relevant added/removed tests
  • packages/one-app-runner/bin/one-app-runner.js
    • '--no-module-map-url': 'modules' addition to implies eliminates need for global require of yargs
    • 'create-docker-network': 'docker-network-to-join' to implies eliminates need for argument validation in startApp.js
    • put it all in an IIFE for cleaner testing
  • packages/one-app-runner/src/startApp.js
    • separated several functions out into utils
    • new util objectToFlags largely eliminates the need for one off arg generation functions and expressions like name ?`--name=${name}` : null
    • moved dockerNetworkToJoin validation to yargs in bin/one-app-runner
    • always send a logStream to spawnAndPipe but make it conditionally stdout instead of piping to stdout when there is no logStream
    • added startJaeger call when includeJaeger is set
  • packages/one-app-runner/src/startJaeger.js
    • pulls the jaeger all-in-one image, spawns it & prettifies/injects logs
    • the PreProcessLogsTransform stream changes the log levels to use numbers instead of strings so that pino-pretty can drop correctly drop levels we don't want logged (info and debug)
    • it also watches for the debug logs generated by the collector when spans are reported
    • unfortunately this includes traces on requests to the jaeger UI, so we fetch from the jaeger API to check that the service name is not one of jaeger's
    • -e=JAEGER_DISABLED should take care of this but it does not. left it in hopes that can be resolved
    • since the debug logs are per span and not per trace we keep track of trace ids we've already handled but forget them after 30 seconds
    • when the service is not jaeger we push a log onto the stream with a link to the trace in the jaeger UI
    • we also collect the trace id from the treaceresponse so we can ignore that as well, preventing us from getting in a loop checking the traces for our own requests.

Motivation

Allow one-app users to view traces locally

Test Conditions

Ran one-app-runner with --include-jaeger with and without --docker-network-to-join
Ran one-app-runner with --include-jaeger and a sub-v6.11.0 version
Ran one-app-runner without --include-jaeger

Validated in that span debug logs and requests to jaeger API are not creating a loop.

Types of changes

Check boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update
  • Security update

Checklist

Check boxes that apply:

  • My code follows the code style of this project.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • I have added the Apache 2.0 license header to any new files created.

@10xLaCroixDrinker 10xLaCroixDrinker marked this pull request as ready for review March 30, 2024 21:19
@10xLaCroixDrinker 10xLaCroixDrinker requested review from a team as code owners March 30, 2024 21:19
@10xLaCroixDrinker 10xLaCroixDrinker merged commit bb5a6be into main Apr 1, 2024
5 checks passed
@10xLaCroixDrinker 10xLaCroixDrinker deleted the feature/jaeger branch April 1, 2024 20:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants