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

feat(#12): add otel service name as an input or env var #13

Conversation

pragmaticivan
Copy link
Contributor

@pragmaticivan pragmaticivan commented Jul 5, 2022

v1v
v1v previously approved these changes Jul 6, 2022
Copy link
Contributor

@nikordaris nikordaris left a comment

Choose a reason for hiding this comment

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

We need to add a test case that validates the service name. You should be able to add the the test in trace.test.ts under the "resource attributes" test suite for a test case of "has service.name resource as otelServiceName" or something like that.

src/tracing/trace.ts Outdated Show resolved Hide resolved
src/tracing/trace.test.ts Outdated Show resolved Hide resolved
@nikordaris
Copy link
Contributor

Thanks for the contribution @pragmaticivan! I left a couple of comments. After those changes i think this will be good to merge in.

@pragmaticivan pragmaticivan force-pushed the feature/add-otel-service-name-support branch from 77faf43 to 0c58ae7 Compare July 24, 2022 16:32
@pragmaticivan
Copy link
Contributor Author

Updated @nikordaris thanks!

nikordaris
nikordaris previously approved these changes Jul 25, 2022
@pragmaticivan
Copy link
Contributor Author

@nikordaris anything I need to fix? I just did run lint and test locally and they are all working.

@pragmaticivan
Copy link
Contributor Author

I've just updated my node version to exactly the one used in the CI (which uses latest and will probably come with unexpected side effects).

By changing the minor version I was able to replicate it locally. Some packages are due with updates, so that might also be a good time to get it in.

@nikordaris
Copy link
Contributor

@pragmaticivan ok, i think i fixed the build issues. if you pull the latest from main it should fix the build for this PR. apparently node 16.16 is stricter on version conflicts than 16.13

@ghost
Copy link

ghost commented Jul 25, 2022

@nikordaris rebased from the main branch.

nikordaris
nikordaris previously approved these changes Jul 25, 2022
@nikordaris
Copy link
Contributor

Ok the update dist action wasn't designed for forks. I need to fix that.

@nikordaris nikordaris dismissed their stale review July 26, 2022 20:07

fixing workflow

@nikordaris
Copy link
Contributor

@pragmaticivan ok i think i resolved the build issues. it looks like it created conflicts for you though. if you pull the latest, hopefully that will fix the build

@pragmaticivan pragmaticivan force-pushed the feature/add-otel-service-name-support branch from 8bc9330 to ec96fbf Compare July 26, 2022 21:48
@pragmaticivan
Copy link
Contributor Author

@nikordaris rebased

@nikordaris nikordaris merged commit 3ed67f9 into inception-health:main Jul 26, 2022
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.

Not able to overwrite service name with OTEL_SERVICE_NAME
3 participants