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 grafana-opentelemetry-starter #1161

Closed
zeitlinger opened this issue Apr 24, 2023 · 10 comments
Closed

Add grafana-opentelemetry-starter #1161

zeitlinger opened this issue Apr 24, 2023 · 10 comments

Comments

@zeitlinger
Copy link

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:

  • loki for logs
  • mimir for metrics
  • tempo for tracing
  • grafana for the whole visualization, dashboards, explore, etc.

Licence

Issue Tracker URL

Continuous integration

Maven Central

Configuration metadata

spring-boot-configuration-processor

Version mappings

The starter supports Boot 3.0.4+

@zeitlinger
Copy link
Author

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

@snicoll
Copy link
Contributor

snicoll commented May 5, 2023

@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
Copy link
Author

friendly ping 😄

@jonatan-ivanov
Copy link

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

  • 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.
  • Why the snapshots repo is needed?
  • 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.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label May 24, 2023
@zeitlinger
Copy link
Author

  • 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 😄

  • 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.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 24, 2023
@jonatan-ivanov
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link
Contributor

snicoll commented Jul 26, 2023

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.

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2023
@snicoll snicoll added status: declined and removed status: feedback-provided Feedback has been provided labels Jul 26, 2023
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

4 participants