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

fix(tracing): Only override the OTEL service name if it is not already set #224

Merged
merged 5 commits into from
Feb 8, 2025

Conversation

tinco
Copy link
Member

@tinco tinco commented Jan 24, 2025

Only override the OTEL service name if it is not already set in OTEL_RESOURCE_ATTRIBUTES or OTEL_SERVICE_NAME.

…y set in OTEL_RESOURCE_ATTRIBUTES or OTEL_SERVICE_NAME
let resource_attributes = std::env::var("OTEL_RESOURCE_ATTRIBUTES")
.unwrap_or_default()
.split(',')
.map(|s| s.split_once("=").expect("invalid OTEL_RESOURCE_ATTRIBUTES"))
Copy link
Member

Choose a reason for hiding this comment

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

So if you do default (empty string), this panics.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh oh no, you're not correct. We first split on , to get the pairs. So if the variable is empty, the split won't yield any results so it would never get to the resource attribute splitter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm you actually are correct, that is surprising..

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it's not really surprising, if you have a list of one element of course it would not include the separator and yield the one result.

Copy link
Member

Choose a reason for hiding this comment

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

I have my moments

@timonv timonv merged commit 47d0928 into master Feb 8, 2025
9 checks passed
@timonv timonv deleted the fix/dont-override-otel-service-name branch February 8, 2025 15:56
This was referenced Feb 8, 2025
timonv pushed a commit that referenced this pull request Feb 9, 2025
## 🤖 New release

* `kwaak`: 0.8.2

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [0.8.2] - 2025-02-09

### 🚀 Features

- Improve line edits and add line editing evaluation (#235)
- Support custom constraints for an agent (#274)
- Optionally hide top header in UI (#275)

### 🐛 Bug Fixes

- *(tracing)* Only override the OTEL service name if it is not already
set (#224)

### ⚙️ Miscellaneous Tasks

- Fmt

<!-- generated by git-cliff -->
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).
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.

2 participants