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

Fix envoy access logs #1930

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mviitane
Copy link
Member

@mviitane mviitane commented Jan 21, 2025

Changes

Fix envoy access logs

Merge Requirements

For new features contributions, please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

- Remove attributes that cause problems in the Collector
@mviitane mviitane requested a review from a team as a code owner January 21, 2025 07:49
Copy link
Contributor

@rogercoll rogercoll left a comment

Choose a reason for hiding this comment

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

Some of these attributes are defined by the OpenTelemetry Semantic Conventions: https://github.com/open-telemetry/semantic-conventions/tree/main/model/http

Would it be feasible to fix the opensearch mappings instead?

@mviitane
Copy link
Member Author

mviitane commented Jan 21, 2025

Some of these attributes are defined by the OpenTelemetry Semantic Conventions: https://github.com/open-telemetry/semantic-conventions/tree/main/model/http

Would it be feasible to fix the opensearch mappings instead?

I know that some of these are in the semantic conventions, but I believe Envoy doesn't send them correctly. I didn't find a good reference configuration and don't know a way to fix these in the Envoy configuration. We can add these attributes back when we get them working.

I don't see how we could fix this with the OpenSearch mapping because the batch processor also shows the same problem.

@rogercoll
Copy link
Contributor

I don't see how we could fix this with the OpenSearch mapping because the batch processor also shows the same problem.

The error is shown in the batch processor because is the error propagated by the OpenSearch exporter, see https://github.com/open-telemetry/opentelemetry-collector/blob/main/processor/batchprocessor/batch_processor.go#L263

@mviitane
Copy link
Member Author

OK, I didn't realize this is only opensearch related. I wonder why the automatic mappings are not enough.
https://opensearch.org/docs/latest/field-types/

@rogercoll
Copy link
Contributor

OK, I didn't realize this is only opensearch related. I wonder why the automatic mappings are not enough. https://opensearch.org/docs/latest/field-types/

Not sure, I am not very familiar with OpenSearch. Have you tried removing the corresponding index? (maybe the http.connection.id mapping was created with some other data)

@mviitane
Copy link
Member Author

OK, I didn't realize this is only opensearch related. I wonder why the automatic mappings are not enough. https://opensearch.org/docs/latest/field-types/

Not sure, I am not very familiar with OpenSearch. Have you tried removing the corresponding index? (maybe the http.connection.id mapping was created with some other data)

I ran docker system prune -a and started a clean environment and got the problem almost right away. Are you not seeing the same problem?

@rogercoll
Copy link
Contributor

OK, I didn't realize this is only opensearch related. I wonder why the automatic mappings are not enough. https://opensearch.org/docs/latest/field-types/

Not sure, I am not very familiar with OpenSearch. Have you tried removing the corresponding index? (maybe the http.connection.id mapping was created with some other data)

I ran docker system prune -a and started a clean environment and got the problem almost right away. Are you not seeing the same problem?

Not on my environment, just tested with nightly DEMO_VERSION=nightly-12898327927 and collector logs are clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collector complains about "Could not dynamically add mapping for field [http.connection.id]"
2 participants