-
Notifications
You must be signed in to change notification settings - Fork 47
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
Updates required to MP Telemetry docs #7655
Comments
Additional @NottyCode feedback from #7657: This page has been written as a task and Liberty docs are not tasks. It is also wrong for a dev topic to link to an ops topic for important context. response from @lauracowen The reason that we care about this is that the approach of adding many tasks in the Liberty IBM Docs has led to many unnecessary topics, bloating the entire set of docs and making it harder to find the information you actually need. That's not to say that the OL docs are perfect by any means, but laying the Liberty IBM Docs format on them like this is not going to help. It mostly comes down to understanding the target audience and ensuring that the information that user audience needs, and only the information that they need, is provided, as concisely as possible. I general, I don't think it's necessarily bad to link to topics for aimed at other users as long as that information is relevant to the user you're currently writing for (and it's clear what info you're expecting them to read there). In this particular case, though, I agree with Alasdair because the developer needs to understand things like application vs runtime-level configuration in the context of the app they're developing and what to choose etc. The current approach of just linking to the OPERATIONS topic is dumping and running - how is the developer meant to decide what to set? The "preparing dev env" topic is fairly uninformative and doesn't need to exist, but the info the developer actually needs isn't available to them. That info should be provided in a topic in the DEVELOPMENT section couched in language that makes sense to the developer in an intro/overview topic called something like "Observability in microservices" in the DEVELOPMENT section. |
additional @NottyCode feedback from #7658 I do not think there is value to this page. I think this would be of more value if it were in the Ops oriented page here |
@dmuelle , the page you're commenting about is something that is only useful to developers, since it's their job to use java.util.logging (or something that routes to JUL) to have their logs included with the mpTelemetry logs. The operations team can't control that. |
@donbourne - sorry I should have attributed that quote properly to @NottyCode . I am reviewing these comments from Alasdair and @lauracowen to come up a with a rewrite. In the case of this particular comment, it the logs info could be included in a revised version of the Development> Observability overview that consolidates conceptual information about developing with OpenTelemetry on Open Liberty. I think Alasdiar and Laura's feedback was generally that the doc is not well served by having a bunch of smaller task topics that mix config and conceptual info. Instead, we could consolidate to topics for Ops and Dev that explain OpenTelemetry as a single solution for logs, trace, and metrics, and point to the feature page for configuration examples. This is more in keeping with the overall content strategy of the open liberty docs. In any case, I will share and discuss any edits before making any public-facing changes. Hoping to contain this work in the 24.0.0.12 release |
some comments on @NottyCode 's original list of comments: Feedback on Enable observability with MicroProfile Telemetry doc:
|
Hi @donbourne @NottyCode @lauracowen - I've revised the following pages per your comments: Please let me know if further edits are needed. So far, I've just worked on the Ops topics. Once this is sorted, I'll work on the dev topics to follow a similar pattern to what we agree upon here w/r/t OpenTelemetry. Thanks again for your feedback. |
@dmuelle , I've just looked at the first doc so far. Here are my comments: adding th following when describing the combination of runtime-level and application-level configuration you should point out that the otel.sdk.disabled property must be set at the application level (otherwise mpTelemetry-2.0 will start in single application mode). OpenTelemtry When you use the otlp default log exporter, the OpenTelemetry Batch LogRecord Processor (BLRP) is enabled and log records are exported in batches according to BLRP default settings. You can can adjust these settings with otel.blrp.* properties. For more information about the available properties and their default settings, see see MicroProfile Config properties: MicroProfile Telemetry.
OpenTelemetry collects both JVM runtime-environment metrics and HTTP metrics.
|
Thanks Don- I've addressed these in the updated draft. For MP Config properties, I've changed this to "system properties" and just mention mp-config in the context of application-level enablement. I also made some edits to the config properties reference page to try and clarify that these are OpenTelemetry properties that are available when you enable MP Telemetry, rather than mp config properties that you specify in mp config properties files. MicroProfile Telemetry: OpenTelemetry properties We also might consider documenting these on a separate page instead of with the MP Config properties. Mp Telemetry is different from ohter mp specs in that it's just exposing another non-MP API so I'm trying to reorient the doc to focus on OpenTelemetry and realy only mention MP Telemetry in the context of enabling the OL feature. |
OpenTelemetry topic
Troubleshooting topic
feature config topic
|
Thanks @lauracowen -
Agreed- maybe "Collect logs, metrics, and traces with OpenTelemetry"? or "Unified observability with OpenTelemetry"? Still thinking
Deleted these
yes
Agreed- I took it out where I could For the "Preparing your development environment..." both these are things you only need to do if ypou want to customize your app for for OpenTelemetry- ie define custom metrics in your app code or manually instrument traces. If you just want to turn it on and collect data without making any changes to your app code, you dont need this stuff. So it seems like it might be more confusing in two separate sections, and we dont want it to get mixed up with the basic prereq of enabling the feature and the system property. It's just about exposing the API and annotations for your app code, if you want to do custom stuff. Maybe a different title would help? "Customize your application telemetry with the OpenTelemetry API" ? |
Yes, maybe a separate section with all the custom stuff in it? And in there make it clear that you don't have to do this for most cases (and give examples or the full scope of where you can just use the default stuff without any custom stuff). That way, they can get up and running as quickly and easily as possible without having to contemplate custom stuff. (I think the word "custom" in the title will help put people off rushing to do it too.) |
Ok, I tried to add more context to make it clear that you only need this config if you want to use the OT API to customize your metrics or trace. I also moved the example down the page so it wouldn't be easily confused with basic enablement: Customize your application telemetry with the OpenTelemetry API |
There's a random |
I have a lot of feedback on the MP Telemetry docs which are here and here.
Feedback on Enable observability with MicroProfile Telemetry doc
Open Telemetry
.This is true of any Liberty feature, so doesn't need to be said here. The last part about needing to enable the Open Telemetry SDK is true, but the following sentence repeats it:
This is also confusing to me because all function is disabled by default because by default no feature is enabled. What it is trying to say is even configuring the feature isn't enough, you also have to set a property but this isn't clear.
otel.service.name
before telling them what that means.otel.service.name
isbootstrap.properties
vsserver.env
. To be honest we should document the one we expect and ignore the other options.otel.exporter.otlp.endpoint=http://localhost:4317/
is mentioned only in the section on trace and yet it is relevant to all, it should have been in the main section on enabling Telemetry. There is no doc on how the collector and exporter concept works either in this page, so we have a setting for a concept we haven't explained.I could write more, but I think this is enough. I would recommend a complete rewrite.
Feedback on MP Open Telemetry Feature Example
otel.sdk.disabled
property to true.The text was updated successfully, but these errors were encountered: