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

Suggest column # as an extra source code attribute #3029

Merged
merged 3 commits into from
Jan 3, 2023

Conversation

jordigh
Copy link
Contributor

@jordigh jordigh commented Dec 12, 2022

Changes

In some languages such as Javascript or Haskell, it's common to have multiple lambdas/anonymous functions defined on the same line.

function myEval(code){return eval(code)}function handleJSONP(a){return a}(function(){function a(b){var c=myEval(b)}})();

Without a column number, it's impossible to accurately pinpoint which of the anonymous functions in the same file and the same line we are talking about. A column attribute is necessary to fully specify which unit of code we are interested in.

@jordigh jordigh requested review from a team December 12, 2022 17:13
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 12, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@jordigh jordigh closed this Dec 12, 2022
@jordigh jordigh reopened this Dec 12, 2022
@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Dec 13, 2022
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

@jordigh Please update the backing YAML files defined in https://github.com/open-telemetry/opentelemetry-specification/tree/main/semantic_conventions as instructed by the README in that folder, this will also make the merge checks pass.

Also, please update the https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md with your addition.

jordigh added 2 commits December 20, 2022 11:11
In some languages such as Javascript or Haskell, it's common to have multiple lambdas/anonymous functions defined on the same line. Without a column number, it's impossible to accurately pinpoint which of the anonymous functions in the same file and the same line we are talking about. A column attribute is necessary to fully specify which unit of code we are interested in.
@jordigh
Copy link
Contributor Author

jordigh commented Dec 20, 2022

@arminru I assume the files that I didn't touch that were missing a type were relying on the default span type. These seem like events to me, but I didn't want to accidentally break something by changing from the existing default. I have made those changes, as well as rebased onto a more recent main.

@Oberon00
Copy link
Member

I don't think you should change anything there. There is only a warning printed which can be ignored.

@jordigh
Copy link
Contributor Author

jordigh commented Dec 21, 2022

I don't think you should change anything there. There is only a warning printed which can be ignored.

It returns nonzero and was showing as a failed build on Github, gotta appease the build gods.

https://github.com/open-telemetry/opentelemetry-specification/actions/runs/3680446681

https://github.com/open-telemetry/opentelemetry-specification/actions/runs/3680446681/jobs/6233690892#step:3:48

@carlosalberto
Copy link
Contributor

@Oberon00 Any last concern?

@Oberon00
Copy link
Member

@carlosalberto No concerns with merging the PR, please go ahead!

(but it is a bit concerning that this error from the convention generator went undetected. Maybe that build job depends on somebody touching the YAML files and nobody did so since the last convention generator update)

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 31, 2022
@Oberon00
Copy link
Member

Oberon00 commented Jan 2, 2023

Hi, I think this should be merged instead of going stale

@carlosalberto carlosalberto merged commit 67e792e into open-telemetry:main Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants