-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore(mock-server): make ui printer optional #165
Conversation
package-lock.json
Outdated
@@ -1,10 +1,12 @@ | |||
{ | |||
"name": "elastic-otel-node", | |||
"version": "0.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for reviewer: at 1st I though this line was added because I was using a different node version but it happened in v14, v16, v18 & v20. Is okay this addition?
* This printer converts to a possible JSON representation of each service | ||
* request and saves it to a file. **Warning**: Converting OTLP service requests to JSON is fraught. | ||
*/ | ||
class FilePrinter extends Printer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for reviewer: detaching the printer from the UI service makes sense. A possible scenario is to store traces for further analysis with other tools. Also changing the usage of jsonStringifyTrace
makes the payload more compact so files are smaller in size
packages/mockotlpserver/lib/cli.js
Outdated
{ | ||
names: ['ui'], | ||
type: 'bool', | ||
help: `Start a web server to inspect traces with some charts.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: don't need backtick string here... though I guess lint doesn't check that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't
|
||
case 'trace-file': | ||
printers.push(new FilePrinter(log)); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the trace-file
"printer" be added the printers
automatically if --ui
is specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One may want to open the UI just to inspect traces from past sessions but I think you question points to the case someone starting the UI but forgetting to put the printer in place, therefore not being able to se traces in the new traces UI. I think is as good point, I'll add it in the next commit
}); | ||
}); | ||
|
||
// Group als spans from the same trace into an ndjson file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
// Group als spans from the same trace into an ndjson file | |
// Group all spans from the same trace into an ndjson file. |
…ic-otel-node into dluna/otlp-server-ui-optin
and also changes the web API to sort traces by date (newest 1st).
Closes: #158