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.

Sign up for GitHub

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

Allow the jaeger exporter path to be configured #2847

Merged
merged 10 commits into from
Feb 24, 2022
+66 −7
Merged

Allow the jaeger exporter path to be configured #2847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6e0ce44
Allow the jaeger exporter path to be configured
Feb 2, 2022
630424c
Do not pull in extra dependencies
Feb 2, 2022
141df78
Move the link in the csproj to the same spot as the rest
Feb 2, 2022
9701f99
Added assertion for the Endoint in the JaegerExporterOptionsTests
Feb 2, 2022
e007798
Merge branch 'main' into add-configurable-path-to-jaeger-exporter
abe545 Feb 10, 2022
9a2eac7
Merge branch 'main' into add-configurable-path-to-jaeger-exporter
abe545 Feb 23, 2022
c12b879
Added change to the changelog
Feb 24, 2022
eb3c20b
Constrain the width to 80 characters to please the linter
Feb 24, 2022
102edb0
Include links in the changelog
Feb 24, 2022
1f7ec6a
Merge branch 'main' into add-configurable-path-to-jaeger-exporter
abe545 Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@
Supported values: `udp/thrift.compact` and `http/thrift.binary` defined
in the [specification](https://github.com/open-telemetry/opentelemetry-specification/blob/9a0a3300c6269c2837a1d7c9c5232ec816f63222/specification/sdk-environment-variables.md?plain=1#L129).
([#2914](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2914))
* Change handling of `OTEL_EXPORTER_JAEGER_ENDPOINT` to require the path to
post. Previous versions of this library would append `/api/traces` to the
value specified in this variable, but now the application author must do so.
This change must also be made if you manually configure the
`JaegerExporterOptions` class - the `Endpoint` must now include the path.
For most environments, this will be `/api/traces`. The effective default
is still `http://localhost:14268/api/traces`. This was done to match
the clarified [specification](https://github.com/open-telemetry/opentelemetry-specification/pull/2333))
([#2847](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2847))

## 1.2.0-rc2

Expand Down
4 changes: 1 addition & 3 deletions src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerHttpClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ public JaegerHttpClient(Uri endpoint, HttpClient httpClient)

this.endpoint = endpoint;
this.httpClient = httpClient;

this.httpClient.BaseAddress = this.endpoint;
}

public bool Connected => true;
Expand Down Expand Up @@ -67,7 +65,7 @@ public int Send(byte[] buffer, int offset, int count)
// Prevent Jaeger's HTTP operations from being instrumented.
using var scope = SuppressInstrumentationScope.Begin();

using HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Post, "/api/traces");
using HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Post, this.endpoint);

request.Content = new ByteArrayContent(buffer, offset, count)
{
Expand Down
6 changes: 4 additions & 2 deletions src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public class JaegerExporterOptions
internal const string OTelAgentHostEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_HOST";
internal const string OTelAgentPortEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_PORT";
internal const string OTelEndpointEnvVarKey = "OTEL_EXPORTER_JAEGER_ENDPOINT";
internal const string DefaultJaegerEndpoint = "http://localhost:14268/api/traces";

internal static readonly Func<HttpClient> DefaultHttpClientFactory = () => new HttpClient();

Expand Down Expand Up @@ -88,9 +89,10 @@ public JaegerExporterOptions()
public int AgentPort { get; set; } = 6831;

/// <summary>
/// Gets or sets the Jaeger HTTP endpoint. Default value: "http://localhost:14268".
/// Gets or sets the Jaeger HTTP endpoint. Default value: "http://localhost:14268/api/traces".
/// Typically https://jaeger-server-name:14268/api/traces.
/// </summary>
public Uri Endpoint { get; set; } = new Uri("http://localhost:14268");
public Uri Endpoint { get; set; } = new Uri(DefaultJaegerEndpoint);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a breaking behavior. Previously /api/traces was appended to the endpoint user provided. Now we expect the user to provide the whole url.

(should be fine as Endpoint was added only in 1.2 and we haven;t yet released a stable)

@CodeBlanch Could you comment?

Also, we should update the readme to reflect the new change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that this a breaking change. As I said in my PR comment, I personally think it makes sense to have this work like the zipkin exporter does - but I totally understand if you would like to avoid a breaking change. This is fairly trivial to change to some other pattern (like maybe another property on the JaegerExporterOptions with the path to append to the endpoint, and have it default to /api/traces).
I'll update the readme as a part of this PR to reflect the decision you make on how to implement this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cijothomas just wanted to check if you've had a chance to discuss with the other maintainers what you'd like me to do here? Thanks!

Copy link
Member

@pellared pellared Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It may be good to confront it with the specification and how other languages are doing it (I suspect inconsistency after a quick look).

PS. OTel Java docs are probably misleading.

Copy link
Contributor Author

@abe545 abe545 Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So in the Environment variable docs it just lists http://localhost:14250 as the default value, and the jaeger docs are explicit about the path to use for Thrift over HTTP: /api/traces. I guess where this breaks down for my organization is that we don't use the jaeger agent, but rather a third party one that fails when we use this combination of values.

I've done some investigation on other otel languages:

  • Java - it looks like it only supports grpc
  • Javascript - Namely is already successfully sending metrics to our agent (It looks like otel js is using the jaeger-client npm package, which just uses the endpoint as-is):
const exporter = new JaegerExporter({
  endpoint: 'http://our-trace-collector:9080/v1/trace',
});
  • Golang - according to their readme and docs, the endpoint can be specified with a path (OTEL_EXPORTER_JAEGER_ENDPOINT defaults to http://localhost:14268/api/traces), and looking through the code, it just uses the endpoint value specified
  • Ruby - readme and code indicate that they export to the URL specified in the env var as-is

I haven't looked through any of the other languages, but it does seem that my change is more consistent with at least a few of the other languages that otel supports (and incidentally all the ones my Company uses 😄).

Copy link
Member

Choose a reason for hiding this comment

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

FYI open-telemetry/opentelemetry-go#2599

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One other thing I forgot to mention is that the behavior in this repo is different from the OpenTracing jaeger client (which we're migrating from). It also just used the endpoint property/env-var as-is.
If you want to keep the behavior as-is, would you have a problem with me adding a new configuration option to set the path? Right now there is no way for me to use jaeger with open telemetry in my organization for .net. That is the problem I'm trying to solve, and I'm not terribly concerned about how to solve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, I opened an issue in the otel spec repo: open-telemetry/opentelemetry-specification#2331

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that it looks like the spec will be updated to make it clear that the endpoint should include the path, how do you feel about this pr @pellared and @cijothomas (also, do I need to do something to kick off the build)?

Copy link
Member

Choose a reason for hiding this comment

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

@abe545 In general I prefer the approach of your PR (this is also how I implemented it in OTel Go SDK some time ago 😄). The change is not really breaking from the perspective as there was no stable release.


/// <summary>
/// Gets or sets the maximum payload size in bytes. Default value: 4096.
Expand Down
2 changes: 1 addition & 1 deletion test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public void JaegerExporterOptions_Defaults()
Assert.Equal(4096, options.MaxPayloadSizeInBytes);
Assert.Equal(ExportProcessorType.Batch, options.ExportProcessorType);
Assert.Equal(JaegerExportProtocol.UdpCompactThrift, options.Protocol);
Assert.Equal(new Uri("http://localhost:14268"), options.Endpoint);
Assert.Equal(JaegerExporterOptions.DefaultJaegerEndpoint, options.Endpoint.ToString());
}

[Fact]
Expand Down
49 changes: 49 additions & 0 deletions test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@
// </copyright>

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using Microsoft.Extensions.DependencyInjection;
using OpenTelemetry.Exporter.Jaeger.Implementation;
using OpenTelemetry.Exporter.Jaeger.Implementation.Tests;
using OpenTelemetry.Resources;
using OpenTelemetry.Tests;
using OpenTelemetry.Trace;
using Thrift.Protocol;
using Xunit;
Expand Down Expand Up @@ -109,6 +112,52 @@ public void ServiceProviderHttpClientFactoryInvoked()
Assert.Equal(1, invocations);
}

[Theory]
[InlineData("/api/traces")]
[InlineData("/foo/bar")]
[InlineData("/")]
public void HttpClient_Posts_To_Configured_Endpoint(string uriPath)
{
// Arrange
ConcurrentDictionary<Guid, string> responses = new ConcurrentDictionary<Guid, string>();
using var testServer = TestHttpServer.RunServer(
context =>
{
context.Response.StatusCode = 200;

using StreamReader readStream = new StreamReader(context.Request.InputStream);

string requestContent = readStream.ReadToEnd();

responses.TryAdd(
Guid.Parse(context.Request.QueryString["requestId"]),
context.Request.Url.LocalPath);

context.Response.OutputStream.Close();
},
out var testServerHost,
out var testServerPort);

var requestId = Guid.NewGuid();
var options = new JaegerExporterOptions
{
Endpoint = new Uri($"http://{testServerHost}:{testServerPort}{uriPath}?requestId={requestId}"),
Protocol = JaegerExportProtocol.HttpBinaryThrift,
ExportProcessorType = ExportProcessorType.Simple,
};

using var jaegerExporter = new JaegerExporter(options);

// Act
jaegerExporter.SetResourceAndInitializeBatch(Resource.Empty);
jaegerExporter.AppendSpan(CreateTestJaegerSpan());
jaegerExporter.SendCurrentBatch();

// Assert
Assert.True(responses.ContainsKey(requestId));
Assert.Equal(uriPath, responses[requestId]);
}

[Fact]
public void JaegerTraceExporter_SetResource_UpdatesServiceName()
{
Expand Down
3 changes: 2 additions & 1 deletion test/OpenTelemetry.Exporter.Jaeger.Tests/OpenTelemetry.Exporter.Jaeger.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Description>Unit test project for Jaeger Exporter for OpenTelemetry</Description>
<TargetFrameworks>netcoreapp3.1;net5.0;net6.0</TargetFrameworks>
Expand Down Expand Up @@ -27,6 +27,7 @@
<Compile Include="$(RepoRoot)\test\OpenTelemetry.Tests\Shared\EventSourceTestHelper.cs" Link="Includes\EventSourceTestHelper.cs" />
<Compile Include="$(RepoRoot)\test\OpenTelemetry.Tests\Shared\TestActivityProcessor.cs" Link="Includes\TestActivityProcessor.cs" />
<Compile Include="$(RepoRoot)\test\OpenTelemetry.Tests\Shared\TestEventListener.cs" Link="Includes\TestEventListener.cs" />
<Compile Include="$(RepoRoot)\test\OpenTelemetry.Tests\Shared\TestHttpServer.cs" Link="Includes\TestHttpServer.cs" />
</ItemGroup>

</Project>