Skip to content

Conversation

@sailalithkanumuri8
Copy link
Contributor

@sailalithkanumuri8 sailalithkanumuri8 commented Jul 21, 2025

For now, I have done the opentelemetry integration for the advanced coffee machine thing. Please let me know if I am in the right path before I proceed to integrate opentelemetry to all things.

There are a lot of file changes in the PR right now because I have merged the docker-debugging branch to this branch. Let me know if I should remove those changed files instead, but having that will be easier to test.

I use a service called Jeager to collect all the opentelemetry logs. When an action on a thing is performed, then opentelemetry will inform Jeager to make a new service and add the logs to it.

In the main.ts file in the advanced coffee thing, the code is just restructured, I didn't change much of the code. I had to do this so that I can do the tracing with opentelemetry for the specific actions.

Please let me know if there is anything I need to change or expand on. Thanks!

@sailalithkanumuri8 sailalithkanumuri8 changed the base branch from main to ege-documentation July 24, 2025 15:42
@sailalithkanumuri8 sailalithkanumuri8 changed the base branch from ege-documentation to main July 24, 2025 15:42
@egekorkan
Copy link
Member

Just talked in the weekly call:

@sailalithkanumuri8
Copy link
Contributor Author

Ok I have made some changes. You can now see the input if a specific service has caused an error. Regarding latency, the tracing service provides basically zero latency because it runs completely async with the actual services being used. Also, when application completely crashes, the spans are lost, but are restarted accordingly to bring it back up when docker engine is running. Anything else like network issues, invalid data, etc, spans are saved and tracing feature logging as no impact.

@egekorkan
Copy link
Member

I think this is a good direction. Some small changes are needed:

  • Make sure to run prettier. This also makes reviewing more difficult
  • You need to add Jaeger as a PR to the infrastructure repository. Otherwise I cannot test it
  • Based on the point above, there should be environment variables for Jaeger (port, subdomain etc.) Currently, everything is hard-coded

Also, any work in node-wot to have deeper span elements in the code?

@sailalithkanumuri8
Copy link
Contributor Author

I am running npm run format, but github actions isn't saying I did... The commits that say "fix: ran npm format" is me running the command, but it isn't changing. Any way you know to fix this? Same for eslint and test.

@danielpeintner
Copy link
Member

I am running npm run format, but github actions isn't saying I did...

Please try to merge in main. Not sure if you created the PR before the prettier toolchain was fixed ...

@danielpeintner
Copy link
Member

Somehow package.json and package-lock.json seem to be out-of-sync
see https://github.com/eclipse-thingweb/test-things/actions/runs/16822623504/job/47652460212?pr=80#step:4:12

@egekorkan
Copy link
Member

@sailalithkanumuri8 merge main into this :)

@sailalithkanumuri8
Copy link
Contributor Author

I have already merged main into this branch. When doing it again right now it says no changes were made. I think there is something wrong with my laptop or something because this keeps happening. Can you try to run npm install? I think it should work for you as when you did it for the docker-debugging PR the package files were up to date and checks passed.

@danielpeintner
Copy link
Member

I have already merged main into this branch. When doing it again right now it says no changes were made. I think there is something wrong with my laptop or something because this keeps happening.

I think you try to merge in your local main. Thats fine, but you need to make sure your local main is in sync. Hence, you might need to update your local main first.

@egekorkan
Copy link
Member

egekorkan commented Aug 12, 2025

Checked out the PR and ran npm install. The lock file got updated. @sailalithkanumuri8 there must be something wrong with your npm/node setup. I did not even merge the main to see this change. Then I have merged the main and the counter-thing folder got updated as expected.

Edit: The linting errors are clearly visible now

@sailalithkanumuri8
Copy link
Contributor Author

Ok I have implemented the changes requested in the call where I reduce redundancy in main.ts in advanced coffee machine and update the readme. Please let me know if there is anything else I can do to get this PR merged!

util/util.ts Outdated
Comment on lines 63 to 66
const getTDJSONSchema = new Promise<Record<string, unknown>>((resolve, reject) => {
https
.get(
"https://raw.githubusercontent.com/w3c/wot-thing-description/main/validation/td-json-schema-validation.json",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok I see. I changed the code so that it uses the wot-thing-description-types now. Please let me know if there is anything else I should change.

README.md Outdated
const tracedThing = createAutoTracedThing(thing);

// Enhanced tracing with injected logic
tracedThing.setPropertyReadHandler("prop", "operation", async (logic: TracedBusinessLogic, options) => {
Copy link
Member

Choose a reason for hiding this comment

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

"operation" shouldn't be needed anymore

| ((logic: TracedBusinessLogic, options?: WoT.InteractionOptions) => Promise<T> | T),
configBuilder?: (builder: TracingConfigBuilder) => TracingConfigBuilder
): void {
let config = tracingConfig(propertyName);
Copy link
Member

Choose a reason for hiding this comment

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

This should also include the operation like read write etc. no?

@egekorkan egekorkan merged commit a029011 into eclipse-thingweb:main Sep 1, 2025
3 of 4 checks passed
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.

3 participants