-
Notifications
You must be signed in to change notification settings - Fork 0
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
DSET-4002: Extract serverHost from the logs and traces #8
DSET-4002: Extract serverHost from the logs and traces #8
Conversation
@martin-majlis-s1 An alternative to that attributes configuration option would be telling the user to configure attribute processor to do that. So instead telling the user to configure our exporter in which attributes it should search for LogRecord server host, we should tell user that This is similar to recommended logstash approach and it gives user the most flexibility - they can populate that attribute with OTel processor / operator / similar as they fit and they can also combine or use multiple processors / operators / etc. In fact, that's also what we do internally with our logstash deployment (we have multiple logstash filters which set logstash event serverHost differently depending on the event and other context). And if we go with that approach, we should include some example operator configuration on how to do that. Also, per discussion in the past and separate ticket, we should also just try to retrieve serverHost from related LogRecord Resource - I think this should work correctly out of the box and cover most common scenarios. WDYT? |
Both approaches have some advantages and disadvantages:
But now when I think about it, I would bet that otel maintainers would also prefer the second version => so I will change it. |
exporter/datasetexporter/README.md
Outdated
@@ -24,6 +24,14 @@ See the [Getting Started](https://app.scalyr.com/help/getting-started) guide. | |||
|
|||
If you do not want to specify `api_key` in the file, you can use the [builtin functionality](https://opentelemetry.io/docs/collector/configuration/#configuration-environment-variables) and use `api_key: ${env:DATASET_API_KEY}`. | |||
|
|||
### Server Host Settings | |||
|
|||
Specifying server host is important for the correct functionality. DataSet expects 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.
I think it would be a good idea to document each specific scenario with config option and example event payload - just to make it more clear to the end user how the DataSet event will look like.
Just to confirm / clarify, extracting I think that's fine, but I think it's better to combine those two PRs together into a single one when opening PR against upstream repo - this should be less work for the open telemetry maintainers since those two changes are related. |
Yes. It will be in the next PR |
@martin-majlis-s1 Thanks. To be able to confirm that |
I have added unit test to verify, that it works as expected. I have also used telemetrygen - and it works as expected: |
@martin-majlis-s1 I think we are not exactly on the same page here. I meant inferring Similar to what do we do with inferring service name for tracing part of the plugin. That's why I said it's important to test it end with the actual otel demo setup. This way we known it works with real work setup and not just manual and specific code path you are exercising. |
Now the serverHost is also extracted from the Resource (as well as |
@martin-majlis-s1 Can you please share DataSet link for the search query above so I can quickly have a look? Thanks. |
The thing is, that only two services are populating logs: |
@martin-majlis-s1 Thanks. I had a quick look and it looks good to me :) P.S. I assume "martin" is hostname which is specified in the config which is used as a default when Resource doesn't contain Did we also test a scenario where it uses collector hostname when that "martin" value is not specified in the config? Thanks. |
I can see, that:
|
Yes. We have unit tests for all scenarios and I have tested it by changing the configuration as well. |
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 didn't get a chance to look into the latest code changes in more detail, but from the end user / outcome perspective, LGTM.
Description: Introduce
server_host
configuration to allow specifying server hostThere is new configuration option
server_host
that allows specifying how to fill the fieldServerHost
of theEvent
structure.New configuration looks like this:
This configuration is applied for logs and traces.
Since it's touching the same code, I have already merged changes from this PR - open-telemetry#23881 - into this branch, hoping that it will be merged soon. It's still not merged, so it's blocked by this.
Link to tracking Issue: open-telemetry#24252 / DSET-4002
Testing:
I have added several unit tests to make sure that it works as expected.
Documentation: There is new section in the README describing the newly added configuration options.