Feature: Generate function and extension logs via Telemetry API receiver #1347

jerrytfleung · 2024-05-25T02:37:28Z

Motivation

Telemetry API can provide function and extension log. It is better to forward logs so as to provide more observability.

Changes

Changed Subscribe function so it takes event type slice in telemetryapi/client.go
Added logs support
Added sharedcomponent (copied from opentelemetry-collector-contrib) so as to share the http server among receivers
Added unit test cases for logs

jerrytfleung · 2024-05-25T02:43:49Z

collector/receiver/telemetryapireceiver/receiver.go

@@ -64,7 +69,7 @@ func (r *telemetryAPIReceiver) Start(ctx context.Context, host component.Host) e
 	}()

 	telemetryClient := telemetryapi.NewClient(r.logger)
-	_, err := telemetryClient.Subscribe(ctx, r.extensionID, fmt.Sprintf("http://%s/", address))
+	_, err := telemetryClient.Subscribe(ctx, []telemetryapi.EventType{telemetryapi.Platform, telemetryapi.Function, telemetryapi.Extension}, r.extensionID, fmt.Sprintf("http://%s/", address))


I wonder if it is a good idea to setup a config that users can choose which type of events to subscribe.
Something like:

receivers: telemetryapi: types: [platform, function, extension]

But after I take a look to config.go. There is a field extensionID. Is it possible to let internal/lifecycle/manager setup the extensionID and users setup the types in the collector configuration?

@tylerbenson @rapphil @nslaughter Can you please help to advise the above question? Thanks!

@jerrytfleung

Sorry about the delay in getting you this review. I appreciate the work you're doing on this. Great stuff!

I'm also not familiar enough with the extension API to say exactly how that will translate to the config from the top of mind.

The config change that you mention seems to me like it will benefit some users and be implemented in a way that's unobjectionable to others, but I think these are questions we'd be best to hash out ~~in a Github issue - ideally accompanied by a very focussed PR that includes the proposed implementation~~ with maintainers, but this comment thread is fine for that discussion.

But after I take a look to config.go. There is a field extensionID. Is it possible to let internal/lifecycle/manager setup the extensionID and users setup the types in the collector configuration

I think we will need to refactor the code a bit so that the extension id is passed to the factory functions createTracesReceiver and createLogsReceiver through the scope.

However, if we are adding configuration, we will also need to be able to support multiple instances of the receivers, which does not seem to be the case with the usage of this SharedComponents abstraction.

After trying out, I found that the extensionID is preserved if I add a field. I updated the code so as to support multiple instances of telemetry api receiver.
I think SharedComponents could help to reuse the http server by using cfg as a key to select the corresponding receiver

tylerbenson · 2024-05-30T18:33:54Z

@rapphil @nslaughter PTAL.

nslaughter

Let me know what you think of the changes suggested in factory.go. That's a key to this PR I think.

nslaughter · 2024-06-14T22:54:23Z

collector/receiver/telemetryapireceiver/receiver.go

+					default:
+					}
+					logRecord.SetSeverityText(logRecord.SeverityNumber().String())
+				}


I haven't implemented much using this part of the OTel logging code, but I think there's some answer to severity parsing challenges in contrib/pkg/stanza. There are also useful constants in plog.

If you don't find a callable solution in the contrib code referenced then it could be sufficient to replace this switch with something like a call to a new helper function func severityTextToNumber(severityText string) SeverityNumber, which just looks up on map[string]SeverityNumber.

Done.
I don't find a callable function in the contrib so I added the function you suggested.

nslaughter · 2024-06-15T04:17:03Z

collector/receiver/telemetryapireceiver/factory.go

+	r := receivers.GetOrAdd(cfg, func() component.Component {
+		return newTelemetryAPIReceiver(cfg, params)
+	})
+	r.Unwrap().(*telemetryAPIReceiver).registerTracesConsumer(next)


I would want more data protection in this file than I see in the top-level variable for receivers, but it looks like the code doesn't read from receivers. As I traced my steps through this code, I started thinking we wouldn't get a benefit from the call to GetOrAdd receivers. What if we changed that to the following?

Suggested change

r := receivers.GetOrAdd(cfg, func() component.Component {

return newTelemetryAPIReceiver(cfg, params)

})

r.Unwrap().(*telemetryAPIReceiver).registerTracesConsumer(next)

r := newTelemetryAPIReceiver(cfg, params)

r.registerTracesConsumer(next)

And if that works we don't have to rely on copying internal/sharedcomponent.

and actually this is a bit fragile. this means that you cannot use this receiver in lets say, more than one pipeline.

I was thinking to reuse the http server. internal/sharedcomponent can help to reuse the http server by looking up the receiver using cfg as a key.
I was assuming we could have configuration for telemetryapi

I think sharedcomponent could play a role so as to reuse http server and to support multiple instances of telemetry api receiver.

Can you take a look to the updated change and let me know your feedback? Thanks!

rapphil · 2024-06-18T18:47:47Z

collector/receiver/telemetryapireceiver/receiver.go

+}
+
+func (r *telemetryAPIReceiver) registerTracesConsumer(next consumer.Traces) {
+	r.nextTraces = next


since we are trying to reuse the http server and it seems that we want to have a single instance of traces consume and logs consumer, would it make sense for nextTraces to be an array?

This would allow to have multiple receivers in different pipelines.

My thinking is to reuse http server within a receiver instance and a receiver instance has a single instance of traces consumer and logs consumers.

I don't know the benefit of making nextTraces to be an array. Can you please elaborate more on that?

rapphil · 2024-06-18T18:57:57Z

My overall comment is that I'm trying to understand why we need to have this:

A dded sharedcomponent (copied from opentelemetry-collector-contrib) so as to share the http server among receivers

Can't we have multiple subscriptions using the telemetry client, each using a different port to subscribe to the events so that we don't have to have this singleton receiver? This will open the door for adding the configurations that you suggested here, which I think is a good idea.

jerrytfleung · 2024-06-20T22:00:02Z

My overall comment is that I'm trying to understand why we need to have this:

Added sharedcomponent (copied from opentelemetry-collector-contrib) so as to share the http server among receivers

Can't we have multiple subscriptions using the telemetry client, each using a different port to subscribe to the events so that we don't have to have this singleton receiver? This will open the door for adding the configurations that you suggested here, which I think is a good idea.

I managed to support multiple instances of telemetry api receiver by allowing user to configure port in the telemetry API receiver configuration. Please help to take a look and let me know your feedback. Thanks!

example config

receivers:
    telemetryapi:
    telemetryapi/1:
      port: 4326
    telemetryapi/2:
      port: 4326
      types:
        - platform
        - function
    telemetryapi/3:
      port: 4326
      types: ["platform", "function"]

jerrytfleung · 2024-06-27T16:24:01Z

@rapphil @nslaughter PTAL. Thanks!

jerrytfleung · 2024-06-27T23:22:32Z

collector/receiver/telemetryapireceiver/receiver.go

+		"FATAL2":   plog.SeverityNumberFatal2,
+		"FATAL3":   plog.SeverityNumberFatal3,
+		"FATAL4":   plog.SeverityNumberFatal4,
+		"CRITICAL": plog.SeverityNumberFatal,


Adding CRITICAL here. It is because Python logging library can log CRITICAL (50) and it is mapped to Fatal in opentelmetry-python https://opentelemetry-python.readthedocs.io/en/latest/_modules/opentelemetry/_logs/severity.html#std_to_otel

jerrytfleung · 2024-06-27T23:23:38Z

collector/receiver/telemetryapireceiver/receiver.go

+		"FATAL3":   plog.SeverityNumberFatal3,
+		"FATAL4":   plog.SeverityNumberFatal4,
+		"CRITICAL": plog.SeverityNumberFatal,
+		"ALL":      plog.SeverityNumberTrace,


Adding ALL here. It is because Java log4j can log ALL and it is mapped to Trace in opentelemetry-java https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/a87245b842a03bc42af6b388bb837fe8884aadae/instrumentation/log4j/log4j-appender-2.17/library/src/main/java/io/opentelemetry/instrumentation/log4j/appender/v2_17/internal/LogEventMapper.java#L201

jerrytfleung

Added comments to explain the decision of add CRITICAL and ALL severity text conversion.

jerrytfleung · 2024-06-27T23:42:11Z

My overall comment is that I'm trying to understand why we need to have this:

A dded sharedcomponent (copied from opentelemetry-collector-contrib) so as to share the http server among receivers

Can't we have multiple subscriptions using the telemetry client, each using a different port to subscribe to the events so that we don't have to have this singleton receiver? This will open the door for adding the configurations that you suggested here, which I think is a good idea.

I am thinking the following configuration so users can configure the types of subscription per each receivers. Also they can put the receiver to the pipeline according to their requirements.

receivers:
  otlp:
    protocols:
      grpc:
        endpoint: "localhost:4317"
      http:
        endpoint: "localhost:4318"
  telemetryapi/1:
    port: 4325
    types: [platform]
  telemetryapi/2:
    port: 4326
    types: [function]
exporters:
  logging:
    loglevel: debug

service:
  pipelines:
    traces:
      receivers: [otlp,telemetryapi/1,telemetryapi/2]
      exporters: [logging]
    metrics:
      receivers: [otlp]
      exporters: [logging]
    logs/1:
      receivers: [telemetryapi/1]
      exporters: [logging]
    logs/2:
      receivers: [telemetryapi/2]
      exporters: [logging]
  telemetry:
    metrics:
      address: localhost:8888

bhaskarbanerjee · 2024-07-12T05:37:31Z

@jerrytfleung I see that the config you shared above is supporting metric in the pipeline as well? metrics:

metrics:
      receivers: [otlp]
      exporters: [logging]

But as far I see this I can only see it supporting traces.

Do you mean that with this PR, the collector will start receiving and exporting metric as well as logs, in addition to the traces?

jerrytfleung · 2024-07-12T16:59:30Z

@jerrytfleung I see that the config you shared above is supporting metric in the pipeline as well? metrics:
metrics:
      receivers: [otlp]
      exporters: [logging]
But as far I see this I can only see it supporting traces.

Do you mean that with this PR, the collector will start receiving and exporting metric as well as logs, in addition to the traces?

@bhaskarbanerjee Solarwinds has plan to extract information from platform event and convert them into metrics. e.g. When we see phase=init from initReport, we can increase the counter of fass.coldstarts and it is a FaaS metrics. Currently, it is still WIP from Solarwinds due to some internal discussion.

This PR covers logs only.

jerrytfleung added 3 commits May 24, 2024 16:44

Added WithLogs and its handling

bd5dd05

nits

63558a8

Added extensions

1ae4fdd

jerrytfleung requested a review from a team as a code owner May 25, 2024 02:37

jerrytfleung commented May 25, 2024

View reviewed changes

jerrytfleung added 2 commits May 27, 2024 17:54

Fixed unit tests

0f74cbe

Added unit test cases

0c54292

tylerbenson added the go Pull requests that update Go code label Jun 13, 2024

nslaughter reviewed Jun 15, 2024

View reviewed changes

rapphil reviewed Jun 18, 2024

View reviewed changes

jerrytfleung added 2 commits June 20, 2024 14:18

Added config (#26)

dd2e317

Merge branch 'main' into feature/telemetryapireceiver-configuration

13a99af

jerrytfleung requested review from rapphil and nslaughter June 20, 2024 22:01

Added severityTextToNumber function

4ff5876

jerrytfleung added 3 commits June 27, 2024 15:29

Corrected README.md

730cfc9

Handled empty types array

999a7ce

Added CRITICAL & ALL

1190cb5

jerrytfleung commented Jun 27, 2024

View reviewed changes

bhaskarbanerjee mentioned this pull request Jul 12, 2024

Otel Collector does not support metric receiver although docs advertise that it does. #1425

Open

Merge branch 'main' into feature/telemetryapireceiver-configuration

165cdda

jerrytfleung and others added 2 commits July 16, 2024 17:26

Merge branch 'main' into feature/telemetryapireceiver-configuration

26a3c68

Removed invalid test case

16f92d1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Generate function and extension logs via Telemetry API receiver #1347

Feature: Generate function and extension logs via Telemetry API receiver #1347

jerrytfleung commented May 25, 2024

jerrytfleung May 25, 2024 •

edited

Loading

jerrytfleung Jun 6, 2024

nslaughter Jun 14, 2024 •

edited

Loading

rapphil Jun 18, 2024

jerrytfleung Jun 20, 2024

tylerbenson commented May 30, 2024

nslaughter left a comment

nslaughter Jun 14, 2024 •

edited

Loading

jerrytfleung Jun 20, 2024

nslaughter Jun 15, 2024 •

edited

Loading

rapphil Jun 18, 2024

jerrytfleung Jun 19, 2024

jerrytfleung Jun 20, 2024

rapphil Jun 18, 2024

jerrytfleung Jun 20, 2024 •

edited

Loading

rapphil commented Jun 18, 2024

jerrytfleung commented Jun 20, 2024 •

edited

Loading

jerrytfleung commented Jun 27, 2024

jerrytfleung Jun 27, 2024

jerrytfleung Jun 27, 2024

jerrytfleung left a comment

jerrytfleung commented Jun 27, 2024

bhaskarbanerjee commented Jul 12, 2024

jerrytfleung commented Jul 12, 2024

Feature: Generate function and extension logs via Telemetry API receiver #1347

Are you sure you want to change the base?

Feature: Generate function and extension logs via Telemetry API receiver #1347

Conversation

jerrytfleung commented May 25, 2024

Motivation

Changes

jerrytfleung May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nslaughter Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerbenson commented May 30, 2024

nslaughter left a comment

Choose a reason for hiding this comment

nslaughter Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nslaughter Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrytfleung Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

rapphil commented Jun 18, 2024

jerrytfleung commented Jun 20, 2024 • edited Loading

jerrytfleung commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrytfleung left a comment

Choose a reason for hiding this comment

jerrytfleung commented Jun 27, 2024

bhaskarbanerjee commented Jul 12, 2024

jerrytfleung commented Jul 12, 2024

jerrytfleung May 25, 2024 •

edited

Loading

nslaughter Jun 14, 2024 •

edited

Loading

nslaughter Jun 14, 2024 •

edited

Loading

nslaughter Jun 15, 2024 •

edited

Loading

jerrytfleung Jun 20, 2024 •

edited

Loading

jerrytfleung commented Jun 20, 2024 •

edited

Loading