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

Clearly state the otel config file is a sample that you need to update in your otel col config — in-app edits #2531

Closed
adnanrahic opened this issue May 12, 2023 · 10 comments
Labels
enhancement New feature or request frontend triage requires triage

Comments

@adnanrahic
Copy link
Contributor

Describe the enhancement you'd like to see
In the in-app settings for OpenTelemetry-based data store integrations:

  • Clearly state the otel config file is a sample that you need to update in your otel col config
  • Mention ingest endpoint and explain what it is
  • Think of a new way to apply the settings without needing to save the setting

In the docs:

  • Write docs pages about otel collector and ingest endpoint for all OpenTelemetry-based data store integrations

Additional context
N/A

@adnanrahic adnanrahic added enhancement New feature or request triage requires triage labels May 12, 2023
@adnanrahic
Copy link
Contributor Author

Hi @olha23 ! Do you have a moment to work on some designs for this?
I'm happy to help with wording and how to get the point across.

Here are some samples we can work with and improve:

Mention ingest endpoint and explain what it is
"Tracetest exposes trace ingestion endpoints on ports 4317 for gRPC and 4318 for HTTP. By toggling the Tracetest ingestion endpoint to 'On', you'll enable ingesting traces into Tracetest. To send traces to Tracetest make sure to use the Tracetest Server's hostname and port. eg. if you are using Docker it would be tracetest:4317 if you are sending traces over gRPC."

Clearly state the otel config file is a sample that you need to update in your otel col config
"The OpenTelemetry Collector configuration below is a sample. Your config file layout should look the same. Make sure to use your own API keys or tokens as explained. Copy the config sample below, paste it into your own OpenTelemetry Collector config and apply it."

@olha23
Copy link

olha23 commented May 15, 2023

@adnanrahic do we need to have also text field here with Tracetest Server hostname and port? or just toggle?

@adnanrahic
Copy link
Contributor Author

If there is a way to expose the hostname of the Tracetest server, then we can also add it. Not sure if it is possible.

Otherwise, just toggle is fine IMHO.

@olha23
Copy link

olha23 commented May 15, 2023

@adnanrahic
Copy link
Contributor Author

I think it looks very nice. We can iterate further on the wording. It can be improved. Let's wait for more feedback from the team.

@olha23 olha23 assigned olha23 and unassigned olha23 May 31, 2023
@olha23
Copy link

olha23 commented May 31, 2023

@kdhamric @xoscar can you guys check the mockups ^ and give your opinion please

@xoscar
Copy link
Collaborator

xoscar commented May 31, 2023

@olha23 my opinion

  • I like the idea of having more information about the otlp endpoints and the change to the save and set text
  • The otlp endpoints are always enabled, we do not have a switch for that in the application, I think it should stay that way, so that switch doesn't make too much sense.

@kdhamric
Copy link
Collaborator

I am torn on the 'Enable' switch. I am afraid this screen is confusing without a value / altering something (ie 'Saving' usually means you altered something, and this screen just seems to present material). We could:

  • disable the 'Save and set as Datastore' button unless the 'Enable Tracetest Ingestion Point' is 'enabled'... this would make the only allowed state for a selected datastore as 'enabled'
    or
  • show an error when clicking 'Save and set as Datastore' if the 'Enable' is not enabled and not allow the action.

Or... we can listen to @xoscar on this one - I am not certain of the best choice and do not feel strongly about it (just afraid it is confusing)

@adnanrahic
Copy link
Contributor Author

I agree some of this does not make sense "technically" as the endpoint is always enabled.

However, I'm thinking about this from the POV of a user. Here's a flow example:

  1. In the data store settings, I want to select a data store.
  2. I have no pre-existing knowledge about the OTLP endpoint.
  3. I have used Jaeger in the provisioning file when starting Tracetest.
  4. I want to switch to using OpenTelemetry as the data store in the in-app settings.
  5. I select the OpenTelemetry data store page, and see that it works by using a trace ingestion endpoint on port 4317/4318.
  6. I read on the page that I need to enable the data store in order to use the ingestion endpoint in my OpenTelemetry config file. (This step requires us to give the user some feedback that an ENABLE or SAVE action needs to happen)

I think that we're okay with not having a toggle, if we write very clear copy and fix the Save and Set as Data Store button to the bottom so that no scrolling is needed.

WDYT?

@kdhamric
Copy link
Collaborator

We have made the screens more clear - closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend triage requires triage
Projects
None yet
Development

No branches or pull requests

4 participants