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

Basic and namedtracer examples do not print traces #1197

Closed
dashpole opened this issue Sep 23, 2020 · 7 comments · Fixed by #1199
Closed

Basic and namedtracer examples do not print traces #1197

dashpole opened this issue Sep 23, 2020 · 7 comments · Fixed by #1199
Labels
area:trace Part of OpenTelemetry tracing
Milestone

Comments

@dashpole
Copy link
Contributor

Both examples appear to be missing traces.

example/namedtracer$ go run main.go
example/basic$ go run main.go
[
	{
		"Name": "ex.com.one{instrumentation.name=ex.com/basic,A=1,B=2,C=3,ex.com/lemons=10}",
		"Min": 1,
		"Max": 1,
		"Sum": 1,
		"Count": 1,
		"Quantiles": [
			{
				"Quantile": 0.5,
				"Value": 1
			},
			{
				"Quantile": 0.9,
				"Value": 1
			},
			{
				"Quantile": 0.99,
				"Value": 1
			}
		]
	},
	{
		"Name": "ex.com.two{instrumentation.name=ex.com/basic,A=1,B=2,C=3,ex.com/lemons=10}",
		"Min": 1.3,
		"Max": 2,
		"Sum": 3.3,
		"Count": 2,
		"Quantiles": [
			{
				"Quantile": 0.5,
				"Value": 2
			},
			{
				"Quantile": 0.9,
				"Value": 2
			},
			{
				"Quantile": 0.99,
				"Value": 2
			}
		]
	}
]

If I add sdktrace.WithSyncer(exp) as an argument to sdktrace.NewProvider here, that fixes the problem for the namedtracer. I can also do the same inside the stdout exporter to fix the basic example.

But looking at the BatchSpanProcessor, it looks like the sdktrace.WithBatcher that does exist should cause traces to be printed.

cc @nilebox

@dashpole
Copy link
Contributor Author

Using WithBatcher prevents correct handling of shutdown for the batch span processor, and should either return the Shutdown() function, or be removed entirely.

The stdout NewExportPipeline and InstallNewPipeline also make it impossible to correctly handle shutdown, as they make use of WithBatcher. Even if we switched to using WithSpanProcessor directly, we need to return the Shutdown function to ensure it can be correctly handled.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 24, 2020

The stdout NewExportPipeline and InstallNewPipeline also make it impossible to correctly handle shutdown.

This sounds like a good thing to address directly as these are useful convenience functions. We should updated these functions (for all exporters) to support returning a generalized shutdown function or the BSP shutdown function directly.

@MrAlias MrAlias added area:trace Part of OpenTelemetry tracing pkg:exporter labels Sep 24, 2020
@MrAlias MrAlias added this to the RC1 milestone Sep 24, 2020
@Aneurysm9
Copy link
Member

The stdout NewExportPipeline and InstallNewPipeline also make it impossible to correctly handle shutdown.

This sounds like a good thing to address directly as these are useful convenience functions. We should updated these functions (for all exporters) to support returning a generalized shutdown function or the BSP shutdown function directly.

Should the apitrace.TracerProvider have a Shutdown() method that can invoke the respective Shutdown() methods of all configured span processors?

@MrAlias
Copy link
Contributor

MrAlias commented Sep 24, 2020

Should the apitrace.TracerProvider have a Shutdown() method that can invoke the respective Shutdown() methods of all configured span processors?

This sounds reasonable to me. This is the main thing operators of instrumentation will interact with so it would follow that it would be a useful place to facilitate all this.

@Aneurysm9
Copy link
Member

Should the apitrace.TracerProvider have a Shutdown() method that can invoke the respective Shutdown() methods of all configured span processors?

This sounds reasonable to me. This is the main thing operators of instrumentation will interact with so it would follow that it would be a useful place to facilitate all this.

Would we need to get a spec update to reflect that new interface?

@dashpole
Copy link
Contributor Author

Should we reopen this to track adding Shutdown(), or open a new issue?

@Aneurysm9
Copy link
Member

Can you open a new issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants