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

Add an OTLP (gRPC) logger to Envoy #13801

Open
itamarkam opened this issue Oct 28, 2020 · 7 comments
Open

Add an OTLP (gRPC) logger to Envoy #13801

itamarkam opened this issue Oct 28, 2020 · 7 comments
Assignees

Comments

@itamarkam
Copy link
Contributor

itamarkam commented Oct 28, 2020

Adding the OTLP logger will allow integration with more 3rd party monitoring frameworks, especially when using OT collector to convert them to whichever format needed.

Although OpenTelemetry logging isn't going to be part of the GA and the C++ SDK isn't ready yet, we don't actually need them in order to export OTLP logs - as long as the OTLP logs proto is stable.
The logs format is actually already available in the Amazon managed OT collector as an exporter, and Microsoft are also working on something similar, so it shouldn't change much - either way we'll get the OT community guarantee for backwards compatibility.

The general idea is to take the existing gRPC logger and just output a different format.

FYI @htuch @yanavlasov

@dio
Copy link
Member

dio commented Oct 30, 2020

Seems like we need to also implement the client-side of collector service: https://github.com/open-telemetry/opentelemetry-proto/tree/master/opentelemetry/proto/collector/logs/v1 (gRPC and HTTP).

@htuch
Copy link
Member

htuch commented Nov 4, 2020

What is meant by "take the existing gRPC logger and just output a different format"? I think OTLP has a totally different gRPC service definition/endpoint as well as data model. I'd suggest making this a dedicated access logger built for OTLP that may borrow concepts (and potentially refactor) with gRPC access logger.

@itamarkam
Copy link
Contributor Author

Some clarifications:

Seems like we need to also implement the client-side of collector service: https://github.com/open-telemetry/opentelemetry-proto/tree/master/opentelemetry/proto/collector/logs/v1 (gRPC and HTTP).

Yes, that's exactly what we need to do.

What is meant by "take the existing gRPC logger and just output a different format"? I think OTLP has a totally different gRPC service definition/endpoint as well as data model. I'd suggest making this a dedicated access logger built for OTLP that may borrow concepts (and potentially refactor) with gRPC access logger.

Yes, the idea is to create a dedicated access logger built for OTLP (i.e implements the client side of the collector service).
I was referring to the implementation details, which I understand how it can be unclear.

As for the implementation, we can re-use functionalities from existing loggers:

  1. gRPC - the existing gRPC HTTP/TCP logger already takes care of streaming, batching etc. Basically I want to use all of this functionality, just with the OT service/protos. I'll start with the gRPC OT service, and if needed we can also implement the HTTP one.
  2. Formatting - the existing File logger has a format config a string/JSON with placeholders to be filled with request/response data. As the OTLP logs are free form, we need the user to configure the output format. I think it's fairly straightforward to extend the string/JSON formatters to support protos as well. That is, the format configuration will be an OTLP log proto with placeholders.

These 2 orthogonal changes can be used to create the gRPC OTLP formattable logger.

The OTLP logger config would look something like this:
{
"common_config": "{...}" // CommonGrpcAccessLogConfig
"otlp_format": "{...}" // OTLP log proto with placeholders, similar to the File logger format config
}

@github-actions
Copy link

github-actions bot commented Dec 9, 2020

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020
@itamarkam
Copy link
Contributor Author

@yanavlasov @htuch Can we tag it with one of the above so it's not closed?

@dio dio added help wanted Needs help! no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Dec 9, 2020
@mattklein123 mattklein123 removed the no stalebot Disables stalebot from closing an issue label Dec 9, 2020
@itamarkam
Copy link
Contributor Author

itamarkam commented Dec 17, 2020

@envoyproxy/security-team @envoyproxy/dependency-shepherds

As part of this issue, I want to add dependencies on some protos from the OT repository

  1. OpenTelemetry collector RPC service
  2. OT logs proto
  3. OT common proto

I'm following the dependency policy doc to get the required approvals.
OpenTelemetry is part of CNCF project, so it should comply with the policy.

Also, I'm not sure what goes into the api/.. dependencies and what goes to the regular one.

@htuch
Copy link
Member

htuch commented Dec 17, 2020

I think it's fine depending on a pure proto repository. The dependency policy largely applies to code that is linked into the data or observability planes, but API proto repos don't need to do things like have a security policy.

This belongs in api/bazel, since the API protos will be depending on it.

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

No branches or pull requests

5 participants