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

remove service.name -> g.co/gae/app/module mapping from trace exporter's default mapping #715

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Aug 23, 2023

Fixes #711

The UI now supports service.name.

This brings the go trace exporter in-line with trace exporters in other languages.

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #715 (373d737) into main (e0f6140) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #715   +/-   ##
=======================================
  Coverage   69.66%   69.67%           
=======================================
  Files          42       42           
  Lines        4840     4838    -2     
=======================================
- Hits         3372     3371    -1     
- Misses       1317     1318    +1     
+ Partials      151      149    -2     
Files Changed Coverage Δ
exporter/trace/trace_proto.go 64.70% <ø> (-0.30%) ⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dashpole dashpole force-pushed the remove_g_co branch 3 times, most recently from ba36ea4 to 7c00afb Compare August 23, 2023 17:55
@dashpole dashpole marked this pull request as ready for review August 23, 2023 18:29
@dashpole dashpole requested a review from a team as a code owner August 23, 2023 18:29
@dashpole dashpole marked this pull request as draft August 25, 2023 17:15
@dashpole dashpole marked this pull request as ready for review August 31, 2023 20:08
Copy link
Contributor

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Is there any concern of this being a breaking change?

@dashpole dashpole changed the title remove g.co* labels from trace exporter now that the UI supports otel semconv remove service.name -> g.co/gae/app/module mapping from trace exporter's default mapping now that the UI supports service.name Sep 22, 2023
@dashpole dashpole changed the title remove service.name -> g.co/gae/app/module mapping from trace exporter's default mapping now that the UI supports service.name remove service.name -> g.co/gae/app/module mapping from trace exporter's default mapping Sep 22, 2023
@dashpole dashpole merged commit 4284c1c into GoogleCloudPlatform:main Sep 22, 2023
25 checks passed
@dashpole dashpole deleted the remove_g_co branch September 22, 2023 19:36
@dashpole
Copy link
Contributor Author

We expect users to prefer service.name, and don't expect many to be using g.co/gae/app/module.

@AndreasBergmeier6176
Copy link

This actually IS a breaking change since g.co/gae/app/module still, at least until January 26th 2024, gets mapped to Service filter.
It would be ok, if the Service filter in GCP Trace would also filter service.name, but it does not!
Thus EVERY developer now needs to get taught to manually search by entering service.name because there is also no hinting for that name in the UI.
Maybe in the future it would be better to sync with the UI team to either improve UX or at least not to break it.

@dashpole
Copy link
Contributor Author

@AndreasBergmeier6176 sorry about that! I've raised it with the UI team, and will report back here when it is fixed. We did work with the UI team on this, but missed the issue you've encountered. Please let me know if you find any other discrepancies caused by this change.

@dashpole
Copy link
Contributor Author

dashpole commented May 1, 2024

@AndreasBergmeier6176 the UI team got back to me, and it isn't an easy problem to fix because of limitations on the trace query language. They said it will likely be fixed as part of broader changes planned for later this year.

In the meantime, you can revert back to the previous behavior with:

trace.New(
	trace.WithAttributeMapping(func(k attribute.Key) attribute.Key {
		switch k {
		case "http.path":
			return "/http/path"
		case "http.host":
			return "/http/host"
		case "http.method":
			return "/http/method"
		case "http.user_agent":
			return "/http/user_agent"
		case "http.status_code":
			return "/http/status_code"
		case "service.name":
			return "g.co/gae/app/module"
		}
		return k
	})
)

Again, very sorry about that.

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.

Remove g.co.* labels from trace export.
3 participants