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

Trace to logs: Add service name and namespace to default tags #71776

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

connorlindsey
Copy link
Contributor

@connorlindsey connorlindsey commented Jul 17, 2023

What is this feature?

Updates the default trace to logs tags to include service.name, service.namespace, and deployment.environment, which are from the OpenTelemetry standard conventions.

Why do we need this feature?

Makes trace to logs work for services instrumented with OTel

Who is this feature for?

OTel users

Which issue(s) does this PR fix?:

Fixes #71767

Special notes for your reviewer:

@connorlindsey connorlindsey added this to the 10.1.x milestone Jul 17, 2023
@connorlindsey connorlindsey requested a review from a team as a code owner July 17, 2023 15:55
@connorlindsey connorlindsey self-assigned this Jul 17, 2023
@connorlindsey connorlindsey requested a review from a team as a code owner July 17, 2023 15:55
@connorlindsey connorlindsey requested review from ashharrison90 and JoaoSilvaGrafana and removed request for a team July 17, 2023 15:55
@github-actions
Copy link
Contributor

Backend code coverage report for PR #71776
No changes

@github-actions
Copy link
Contributor

github-actions bot commented Jul 17, 2023

Frontend code coverage report for PR #71776

Plugin Main PR Difference
explore 86.41% 86.41% 0%

@connorlindsey connorlindsey marked this pull request as draft July 17, 2023 17:37
@cyrille-leclerc
Copy link
Contributor

cyrille-leclerc commented Jul 17, 2023

I'm not clear on the role of these tags but would it be relevant to also add deployment.environment if we add service.namespace and service.name?

By the way, shall we update the documentation https://grafana.com/docs/grafana/next/datasources/tempo/#trace-to-logs as it is listing those default tags?

@joey-grafana
Copy link
Contributor

This looks good overall, thanks for creating the PR.

This will render the Related logs button in the trace view for a row even if the user has not configured any trace to logs tags in their data source configuration. This allows us to guess some of the tags that may be present in the users logs configuration and if they are, then we allow them to link from their trace to the relevant logs. Connor what do you think about adding deployment.environment too.. is this something that you see commonly used?

Also, I would replace line 152 in createSpanLink with the following const tagsToUse = traceToLogsOptions.tags && traceToLogsOptions.tags.length > 0 ? traceToLogsOptions.tags : defaultKeys; as it won't work if the user has previously selected tags but has since deleted them as traceToLogsOptions.tags will not be undefined, it will simply be an empty array.

Screenshot 2023-07-19 at 16 07 43 Screenshot 2023-07-19 at 16 02 59

@joey-grafana
Copy link
Contributor

Ohh yes also we should update the default tags referenced in the docs and the tooltip in the configuration page to match exactly with the default tags we've added in our code.

@cyrille-leclerc
Copy link
Contributor

cyrille-leclerc commented Jul 19, 2023

Thanks @joey-grafana . View related logs is a great feature.

@connorlindsey I think we will benefit of deployment.environment as well as it's what we recommend to segregate production and non production environments. deployment.environment is recommended in our integration tiles...

@connorlindsey
Copy link
Contributor Author

@joey-grafana Thanks for the feedback. We may actually have to use a custom query for the plugin, so setting the default tags may not work (although this could still be a valid improvement for Explore). If that's the case, we'll handle it by transforming the dataframes in the plugin, but I'll still finish this PR.

@connorlindsey connorlindsey marked this pull request as ready for review July 26, 2023 16:33
@github-actions github-actions bot added the pr/external This PR is from external contributor label Jul 26, 2023
Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

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

Looks good, apart from one suggestion.. thank you for the updates :)

public/app/features/explore/TraceView/createSpanLink.tsx Outdated Show resolved Hide resolved
@joey-grafana joey-grafana modified the milestones: 10.1.x, 10.2.x Jul 27, 2023
@joey-grafana
Copy link
Contributor

@connorlindsey just updated the milestone :)

@connorlindsey connorlindsey merged commit f3b6e7d into main Jul 27, 2023
15 checks passed
@connorlindsey connorlindsey deleted the connor/t2l-tags branch July 27, 2023 16:31
sarahzinger pushed a commit that referenced this pull request Aug 1, 2023
* Add service name and namespace to default trace to logs tags

* Add deployment.environment. Update docs

* Revert metrics query tags type
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
…a#71776)

* Add service name and namespace to default trace to logs tags

* Add deployment.environment. Update docs

* Revert metrics query tags type
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trace to Logs: Update default tags to include OpenTelemetry semantic attributes
5 participants