Skip to content

fix: rideshare tweaks #4075

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Conversation

shelldandy
Copy link
Contributor

@shelldandy shelldandy commented Apr 3, 2025

Continues from: #4011

Needed to add the utf8 metrics to our prometheus config so they're properly scrapped

image

@shelldandy shelldandy marked this pull request as ready for review April 3, 2025 22:09
@shelldandy shelldandy requested a review from a team as a code owner April 3, 2025 22:09
@shelldandy shelldandy requested a review from marcsanmi April 5, 2025 02:54
Copy link
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing and it'd be ready to merge 🙂

@shelldandy shelldandy requested a review from marcsanmi April 14, 2025 14:12
Copy link
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @shelldandy,
I still see the a.utf8.metric being added. It's breaking the experience in some screens, such as in the metrics explorer, as I showed in the last comment.

When addressing that, could you take the chance to update the branch? Thanks for all the work! 🙂

@shelldandy
Copy link
Contributor Author

Hi @shelldandy,

I still see the a.utf8.metric being added. It's breaking the experience in some screens, such as in the metrics explorer, as I showed in the last comment.

When addressing that, could you take the chance to update the branch? Thanks for all the work! 🙂

The metric is still there but without the emoji like before, is that still a problem? I'm only using normal characters in there.

I'll update the branch as well no problem

@marcsanmi
Copy link
Contributor

The metric is still there but without the emoji like before, is that still a problem? I'm only using normal characters in there.

Yes, "." aren't supported :(

432264911-2714eb0c-700b-4d51-94db-1a7794a223d6

@shelldandy shelldandy requested a review from marcsanmi April 16, 2025 22:51
Copy link
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last fix and we should be able to merge 🙂

Please take the chance to update the branch from main, sorry about this.

@@ -358,12 +326,22 @@ func MeterProvider(c Config) (*sdkmetric.MeterProvider, error) {
}

ctx := context.Background()
exp, err := otlpmetrichttp.New(ctx)
opts := []otlpmetrichttp.Option{otlpmetrichttp.WithEndpoint(c.OTLPUrl)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found another issue. I think this is the culprit causing this, since it overrides the OTEL_EXPORTER_OTLP_METRICS_ENDPOINT environment variable configuration:

2025/04/23 07:47:43 failed to upload metrics: Post "http:///api/v1/otlp/v1/metrics": http: no Host in request URL

After the changes, try to run the example to see if the otlp metrics added in this PR #3905 are still available.

Copy link
Contributor

@marcsanmi marcsanmi Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still seeing the error above when running the example with docker-compose up --build. Could you please take another look? @shelldandy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be working properly now, please take another look @marcsanmi

The code was previously only using OTLP_URL for metrics endpoint configuration,
which could lead to "no Host in request URL" errors when using OpenTelemetry
standard environment variables. This change adds support for the standard
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT environment variable while maintaining
backward compatibility with OTLP_URL.
@shelldandy shelldandy requested a review from marcsanmi May 7, 2025 20:26
@marcsanmi
Copy link
Contributor

Hi @shelldandy, I'm still not seeing e.g. the handler_duration_seconds_bucket_metric. I've changed the MeterProvider instead of using the ops to be:

	ctx := context.Background()
	exp, err := otlpmetrichttp.New(ctx)
	if err != nil {
		return nil, err
	}

And it seems to work. I can see both, metrics from #3905 as well as your new utf8 metrics. Would this serve your purpose?

Note: For following reviews please ask anyone in @\grafana/pyroscope-team since I'll be out for some weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants