search

spring-io / start.spring.io

https://start.spring.io
Apache License 2.0
2.24k stars 906 forks source link

Add grafana-opentelemetry-starter #1161

Closed zeitlinger closed 1 year ago

zeitlinger commented 1 year ago

Hello,

I'm Gregor - working OpenTelemetry Java instrumentation at Grafana Labs.

The Grafana LGTM stack is a popular observability solution for OpenTelemetry, supporting logs, metrics, and tracing (available as OSS, Enterprise and Cloud).

The grafana-opentelemetry-starter bundles together

Well-established project with a vibrant community

The starter itself is new, but the projects that comprise the LGTM stack have vibrant communities:

Licence

Issue Tracker URL

Continuous integration

Maven Central

Configuration metadata

spring-boot-configuration-processor

Version mappings

The starter supports Boot 3.0.4+

zeitlinger commented 1 year ago

Hi, is there anything I can do or that is missing?

snicoll commented 1 year ago

@zeitlinger I don't know as we haven't got the time to review the proposal yet. Sorry, and thank you for your patience.

zeitlinger commented 1 year ago

friendly ping :smile:

jonatan-ivanov commented 1 year ago

My two cents/questions (sorry for the delay):

zeitlinger commented 1 year ago
  • Is this starter considered stable? From the version number, it seems so but I'm not sure I see how the -alpha versions will not break the users in the future.

alpha is needed for internal configuration - the user visible configuration (in application.properties) is controlled by us, so we can keep it backwards compatible. I'm happy to dive into the details - but it's a longer explanation :smile:

  • Why the snapshots repo is needed?

This was from early local testing - can be removed if necessary

  • Micrometer has OTLP support, I'm not sure I understand why the OTel Metrics SDK (Micrometer bridge) is needed (basically Observation API -> Micrometer Metrics API -> Registry vs. Observation API -> Micrometer Metrics API -> Micrometer Bridge (registry) -> OTel SDK -> OTel exporter), it seems to me that it introduces unnecessary complexity.

support for headers was missing in boot 3.0. With could create a new version targeting 3.1 to remove this complexity. It would take away the possibility to use the otel env vars, but I'm not sure how much user value this feature.

jonatan-ivanov commented 1 year ago

alpha is needed for internal configuration - the user visible configuration (in application.properties) is controlled by us, so we can keep it backwards compatible. I'm happy to dive into the details - but it's a longer explanation

I'm not sure this is true for logging, users need to interact with the OTel appender, don't they? There is not layer between the users and OTel that would prevent them experiencing breaking changes, right?

This was from early local testing - can be removed if necessary

I think it would make things safer (accidentally resolving something that you don't want to) but I guess the published artifacts what matters eventually.

support for headers was missing in boot 3.0. With could create a new version targeting 3.1 to remove this complexity.

I would say support for headers was missing in Micrometer 1.10. You can use Micrometer 1.11 with Boot 3.0 (it's a drop-in replacement) and set-up headers from java (instead of a property), should still be a one-liner).

It would take away the possibility to use the otel env vars, but I'm not sure how much user value this feature.

The OTLP registry does support OTel env vars. If users report that something is missing, we can add them but for Boot users I would highly recommend using Boot properties instead of OTel env vars.

zeitlinger commented 1 year ago

I'm not sure this is true for logging, users need to interact with the OTel appender, don't they? There is not layer between the users and OTel that would prevent them experiencing breaking changes, right?

The main issue with logs was the stability of the SDK - which is stable now. The appender is from the java agent repo - and therefore is still in alpha.

We could make a GrafanaAppender that simply extends OpenTelemetryAppender to guard against incompatible changes in the appender (e.g. a renamed property) - not sure if that's necessary.

support for headers was missing in boot 3.0. With could create a new version targeting 3.1 to remove this complexity.

I would say support for headers was missing in Micrometer 1.10. You can use Micrometer 1.11 with Boot 3.0 (it's a drop-in replacement) and set-up headers from java (instead of a property), should still be a one-liner).

OK, didn't know.

It would take away the possibility to use the otel env vars, but I'm not sure how much user value this feature.

The OTLP registry does support OTel env vars. If users report that something is missing, we can add them but for Boot users I would highly recommend using Boot properties instead of OTel env vars.

OK, I'll make a version that's using this approach - would it make sense to call it "3.1" to align with the boot version?

jonatan-ivanov commented 1 year ago

The main issue with logs was the stability of the SDK - which is stable now. The appender is from the java agent repo - and therefore is still in alpha.

The page you linked is about the specification of the SDK (markdown file) not the implementation (code/artifacts in Maven Central). I guess what the users will interact with is the logback and log4j2 appenders (they are alpha right now). I think we should ask the Java SIG about them because maybe they are effectively stable just in the wrong place right now (moving should be ok/relatively quick I guess) and/or maybe there are plans to make the user-facing things stable. Based on the SDK spec being stable, I'm positive that the logging implementation pieces will stabilize pretty quickly.

Oh one more thing: since boot supports both logback and log4j2, I think adding a short section to the docs how to replace the OTel logback support to the OTel log4j support might be a good idea (exclude the former, include the latter).

We could make a GrafanaAppender that simply extends OpenTelemetryAppender to guard against incompatible changes in the appender (e.g. a renamed property) - not sure if that's necessary.

I think that could be a good idea but not sure if that's necessary either: let's ask the Java SIG first about the stability of the user-facing bits that the starter is depending on.

would it make sense to call it "3.1" to align with the boot version?

I think that could seem to be a good idea at first sight but following the Boot versions could be an unnecessary burden you might not want to get into. There are some starter projects that are following the Boot major version (e.g.: mybatis-spring-boot-starter), I guess mostly because they are following along since 1.x (I'm not aware if anyone would match the minor versions though). There are also starters who does not match the major versions (e.g: solace-spring-boot-starter), I think 1.x for Boot 3.x should be totally fine but I'll let others comment on that.

zeitlinger commented 1 year ago

The page you linked is about the specification of the SDK (markdown file) not the implementation (code/artifacts in Maven Central). I guess what the users will interact with is the logback and log4j2 appenders (they are alpha right now). I think we should ask the Java SIG about them because maybe they are effectively stable just in the wrong place right now (moving should be ok/relatively quick I guess) and/or maybe there are plans to make the user-facing things stable. Based on the SDK spec being stable, I'm positive that the logging implementation pieces will stabilize pretty quickly.

good idea - added it to the agenda next week. I've already seen that the Java SDK in in RC and will become stable next month.

Oh one more thing: since boot supports both logback and log4j2, I think adding a short section to the docs how to replace the OTel logback support to the OTel log4j support might be a good idea (exclude the former, include the latter).

I think we can just include both since the appenders are very lightweight.

snicoll commented 1 year ago

This request has been opened for quite some time and we need feedback provided by @jonatan-ivanov to be processed before we can consider this again. I am going to close this now as we've discussed this internally and we'd like the integration to be more consistent with other similar entries first.