-
Notifications
You must be signed in to change notification settings - Fork 108
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
Introduce JSON span printer #1035
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1035 +/- ##
===========================================
+ Coverage 67.49% 81.77% +14.28%
===========================================
Files 139 139
Lines 11142 11289 +147
===========================================
+ Hits 7520 9232 +1712
+ Misses 3112 1541 -1571
- Partials 510 516 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Great first PR! I left a few comments, mostly about naming and tests.
I think we should modify one of our YAML integration tests to use BEYLA_PRINT_TRACES_JSON
and another which uses both BEYLA_PRINT_TRACES_JSON
and BEYLA_PRINT_TRACES_JSON_INDENT
.
@mariomac do you think we should handle the NetO11y printer in this PR or we should follow-up with another PR? Perhaps the question is, do we want structured logging for network traces? |
@grcevski as you prefer but I think it's fine to handle it in another PR |
a0a782c
to
c9c09c5
Compare
|
@grcevski I still need to update the relevant docs, will do it very soon |
As a design improvement, I'd maybe add a breaking change:
Another option to avoid the breaking change could be to keep supporting |
@mariomac I'd lean towards the |
Removed code duplication as suggested by @marctc - cmdline option and docs change will follow. The new JSON looks like this:
|
ffdd9a2
to
e31851c
Compare
Documentation changes coming up next. |
@@ -223,11 +226,26 @@ func (c *Config) Validate() error { | |||
" purposes, you can also set BEYLA_NETWORK_PRINT_FLOWS=true") | |||
} | |||
|
|||
if !c.TracePrinter.Valid() { |
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.
Can we add tests in config_test.go for this logic here?
pkg/internal/request/span_test.go
Outdated
|
||
require.NoError(t, err) | ||
|
||
assert.Equal(t, s["type"], "HTTP") |
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.
I think we can make this simpler by checking in one go, we have examples throughout the code, for example:
assert.Equal(t, map[string]map[string]string{
"/user/1234": {
string(semconv.ServiceNameKey): "svc-1",
string(semconv.ServiceNamespaceKey): "ns",
string(attr.ClientAddr): "1.1.1.1",
string(attr.HTTPRequestMethod): "GET",
string(attr.HTTPResponseStatusCode): "201",
string(attr.HTTPUrlPath): "/user/1234",
string(attr.ServerPort): "8080",
string(attr.ServerAddr): events["/user/1234"]["server.address"],
},
"/user/4321": {
string(semconv.ServiceNameKey): "svc-3",
string(semconv.ServiceNamespaceKey): "ns",
string(attr.ClientAddr): "1.1.1.1",
string(attr.HTTPRequestMethod): "GET",
string(attr.HTTPResponseStatusCode): "203",
string(attr.HTTPUrlPath): "/user/4321",
string(attr.ServerPort): "8080",
string(attr.ServerAddr): events["/user/1234"]["server.address"],
},
}, events)
Thanks, now looks much better! I was envisioning a conversion from OTEL span (not internal one) to JSON, but maybe that's way to complicated. |
pkg/internal/request/span.go
Outdated
@@ -63,36 +112,184 @@ type PidInfo struct { | |||
// REMINDER: any attribute here must be also added to the functions SpanOTELGetters, | |||
// SpanPromGetters and getDefinitions in pkg/internal/export/metric/definitions.go |
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.
I just checked and this file doesn't exist anymore. We can either remove this coment or point to another file. Maybe @mariomac can clarify here.
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.
Oh, yeah. It was moved to: pkg/export/attributes/attr_defs.go
.
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.
I wish Go had a Javadoc-like way of referencing functions and files.
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.
@rafaelroquetto would you mind changing those comments? thanks!
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.
@marctc sorry, totally missed it - updated.
69dc012
to
e4249a5
Compare
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.
This is looking really good now. I think all we need to add is:
- Some integration tests should use the "json" and some "json_indent" printer, to ensure we don't crash with those options.
- Documentation.
37c6968
to
061648c
Compare
docs/sources/quickstart/cpp.md
Outdated
@@ -51,13 +51,13 @@ To run Beyla, first set the following environment variables: | |||
- The `OTEL_EXPORTER_OTLP_PROTOCOL`, `OTEL_EXPORTER_OTLP_ENDPOINT` and `OTEL_EXPORTER_OTLP_HEADERS` variables copied from the previous step. | |||
- `BEYLA_OPEN_PORT`: the port the instrumented service is using (for example, `80` or `443`). If using the example service in the first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. This will cause Beyla to print traces to standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
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.
@grcevski is this something that should be addressed as part of this PR?
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.
Yes 100%. You can reword it as something like:
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. When this option is set, Beyla will print traces in text format to the standard output.
docs/sources/quickstart/nodejs.md
Outdated
@@ -53,7 +53,7 @@ To run Beyla, first set the following environment variables: | |||
(for example, `80` or `443`). If using the example service in the | |||
first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. This will cause Beyla to print traces to standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/golang.md
Outdated
@@ -59,13 +59,13 @@ To run Beyla, first set the following environment variables: | |||
(for example, `80` or `443`). If using the example service in the | |||
first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. This will cause Beyla to print traces to standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/python.md
Outdated
@@ -51,7 +51,7 @@ To run Beyla, first set the following environment variables: | |||
(for example, `80` or `443`). If using the example service in the | |||
first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. This will cause Beyla to print traces to standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/ruby.md
Outdated
@@ -51,7 +51,7 @@ To run Beyla, first set the following environment variables: | |||
(for example, `80` or `443`). If using the example service in the | |||
first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. This will cause Beyla to print traces to standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/configure/options.md
Outdated
|
||
<a id="printer"></a> | ||
|
||
Prints any instrumented trace on the standard output (stdout). The value of |
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.
🚫 [vale] reported by reviewdog 🐶
[Grafana.Spelling] Did you really mean 'stdout'? The Grafana dictionary might not know of this word yet. To add a new word, refer to Add words to the Grafana Labs dictionary. Alternatively, raise an issue and a maintainer will add it for you. For UI elements, use bold formatting. The spell checker doesn't check words with bold formatting. For paths; configuration; user input; code; class, method, and variable names; statuscodes; and console output, use code formatting. The spell checker doesn't check words with code formatting.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
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.
@grcevski I might need some help here
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.
No reason to keep (stdout).
docs/sources/configure/options.md
Outdated
| `json_indent` | prints an indented JSON object | | ||
|
||
|
||
**The following option has been DEPRECATED.** *Use `trace_printer` instead.* |
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.
I've added this to be explicit about the deprecation - not sure if this is what we usually do, so happy to remove it otherwise.
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.
No need to mention the deprecation, we should only have documents for the new option, for the folks already using PRINT_TRACES=1 things will work as usual.
7614fa9
to
055c93c
Compare
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.
Great stuff! Amazing first PR 🚀
docs/sources/quickstart/cpp.md
Outdated
@@ -51,13 +51,13 @@ To run Beyla, first set the following environment variables: | |||
- The `OTEL_EXPORTER_OTLP_PROTOCOL`, `OTEL_EXPORTER_OTLP_ENDPOINT` and `OTEL_EXPORTER_OTLP_HEADERS` variables copied from the previous step. | |||
- `BEYLA_OPEN_PORT`: the port the instrumented service is using (for example, `80` or `443`). If using the example service in the first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. When this option is set, Beyla will print traces in text format to the standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/nodejs.md
Outdated
@@ -53,7 +53,7 @@ To run Beyla, first set the following environment variables: | |||
(for example, `80` or `443`). If using the example service in the | |||
first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. When this option is set, Beyla will print traces in text format to the standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/java.md
Outdated
@@ -47,13 +47,13 @@ To run Beyla, first set the following environment variables: | |||
- The `OTEL_EXPORTER_OTLP_PROTOCOL`, `OTEL_EXPORTER_OTLP_ENDPOINT` and `OTEL_EXPORTER_OTLP_HEADERS` variables copied from the previous step. | |||
- `BEYLA_OPEN_PORT`: the port the instrumented service is using (for example, `80` or `443`). If using the example service in the first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This causes Beyla to print traces to the standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. When this option is set, Beyla will print traces in text format to the standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/ruby.md
Outdated
@@ -51,7 +51,7 @@ To run Beyla, first set the following environment variables: | |||
(for example, `80` or `443`). If using the example service in the | |||
first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. When this option is set, Beyla will print traces in text format to the standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/python.md
Outdated
@@ -51,7 +51,7 @@ To run Beyla, first set the following environment variables: | |||
(for example, `80` or `443`). If using the example service in the | |||
first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. When this option is set, Beyla will print traces in text format to the standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/golang.md
Outdated
@@ -59,13 +59,13 @@ To run Beyla, first set the following environment variables: | |||
(for example, `80` or `443`). If using the example service in the | |||
first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This will cause Beyla to print traces to standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. When this option is set, Beyla will print traces in text format to the standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
docs/sources/quickstart/rust.md
Outdated
@@ -47,13 +47,13 @@ To run Beyla, first set the following environment variables: | |||
- The `OTEL_EXPORTER_OTLP_PROTOCOL`, `OTEL_EXPORTER_OTLP_ENDPOINT` and `OTEL_EXPORTER_OTLP_HEADERS` variables copied from the previous step. | |||
- `BEYLA_OPEN_PORT`: the port the instrumented service is using (for example, `80` or `443`). If using the example service in the first section of this guide, set this variable to `8080`. | |||
|
|||
To facilitate local testing, set the `BEYLA_PRINT_TRACES=true` environment variable. This causes Beyla to print traces to the standard output. | |||
To facilitate local testing, set the `BEYLA_TRACE_PRINTER=text` environment variable. When this option is set, Beyla will print traces in text format to the standard output. |
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.
[Grafana.GoogleWill] Avoid using 'will'. Use present tense for statements that describe general behavior that's not associated with a particular time.
For more information, refer to https://developers.google.com/style/tense.
If the rule is incorrect or needs improving, report an issue.
If you have reason to diverge from the style guidance, to skip a rule, refer to Skip rules.
7254080
to
4d72510
Compare
@rafaelroquetto I just wanted to point out that tests are failing. There's some problem with the code you've added:
|
The marshalled JSON object features a different layout in comparison to the original Span struct.
d9b9ad2
to
2943c9a
Compare
Introduce a new flag called
BEYLA_TRACE_PRINTER
, deprecateBEYLA_PRINT_TRACES
, and removeBEYLA_NOOP_TRACES
.The new
BEYLA_TRACE_PRINTER
supersedes bothBEYLA_PRINT_TRACES
andBEYLA_NOOP_TRACES
. It also adds two new output modes,json
andjson_indent
as described below.BEYLA_TRACE_PRINTER
accepts the following values:text
equivalent toBEYLA_PRINT_TRACES=true
: prints a text line with the span datacounter
equivalent toBEYLA_TRACES_NOOP=true
: prints the number of processed requests at the end of executionjson
prints the span as a compact JSON documentjson_indent
prints the span as an indented JSON documentdisabled
- equivalent toBEYLA_PRINT_TRACES=false
Closes #344
TODO