add optional parameter to record_exception method

[shreyagupta30]

This fixes #1102.
I have added two additional attributes to record_exception method. Those are

attributes: types.Attributes = None,
timestamp: typing.Optional[int] = None,

I have applied the tox lint checks.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Shreya Gupta (63f57b6)

[codeboten]

Thanks @shreyagupta30 for the PR, the same change will be needed in the SDK here:

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

Line 661 in c782f14

def record_exception(self, exception: Exception) -> None:

Please add an entry to opentelemetry-api/changelog and opentelemetry-sdk/changelog to note the change.

[shreyagupta30]

@codeboten I have made required changes but somehow almost all the tests are failed. What do you suggest me to do now?

[codeboten]

Looks like a missing import: https://github.com/open-telemetry/opentelemetry-python/pull/1242/checks?check_run_id=1255343757#step:6:578

[shreyagupta30]

@codeboten kindly let me know if any further changes are required.

[codeboten]

Thanks for addressing my previous comments, please add a test to ensure that attributes and timestamp parameters are used when record_exception is called with the additional parameters.

[codeboten]

the additional parameters here (attributes, timestamp) should be used in the method below. As per the spec:

If attributes with the same name would be generated by the method already, the additional attributes take precedence

[shreyagupta30]

@codeboten Can you please guide me on how to do this. I am not able to figure out what exactly needs to be done here.

[codeboten]

@shreyagupta30 you'll want to pass in the timestamp argument into add_event, you can see what it looks like by looking at the method signature here:

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

Lines 589 to 593 in 937863f

	def add_event(
	self,
	name: str,
	attributes: types.Attributes = None,
	timestamp: Optional[int] = None,

You'll also want to pass the attributes parameter in the add_event call below, ensuring that any attributes passed into this method overrides any of the default attributes, the way i'd approach it is probably to create a dict with the default attributes seen below, then loop through the attributes arg and set any values in the dict that way.

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

Lines 697 to 701 in 937863f

	attributes={
	"exception.type": exception.__class__.__name__,
	"exception.message": str(exception),
	"exception.stacktrace": stacktrace,
	},

Last but not least, adding a test like the one below to ensure the attributes are set as expected

opentelemetry-python/opentelemetry-sdk/tests/trace/test_trace.py

Lines 855 to 872 in 937863f

	def test_record_exception(self):
	span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
	try:
	raise ValueError("invalid")
	except ValueError as err:
	span.record_exception(err)
	exception_event = span.events[0]
	self.assertEqual("exception", exception_event.name)
	self.assertEqual(
	"invalid", exception_event.attributes["exception.message"]
	)
	self.assertEqual(
	"ValueError", exception_event.attributes["exception.type"]
	)
	self.assertIn(
	"ValueError: invalid",
	exception_event.attributes["exception.stacktrace"],
	)

[toumorokoshi]

+1 to @codeboten's PR: the SDK is not honoring the intended behavior and adding the event.

[codeboten]

@shreyagupta30 will you have time to look at this PR, the issue it's addressing is time sensitive.

[shreyagupta30]

I am so sorry @codeboten, I got busy with other work. I will definitely get back to it. I was looking into unit tests as I am not very familiar with it.

[codeboten]

No worries @shreyagupta30, if you're ok with it, we can merge this PR as is and then get the other PR #1314 committed over top of it to address the unit tests and implementation details.

[shreyagupta30]

Sure @codeboten Thanks a lot :D

[toumorokoshi]

if you're ok with it, we can merge this PR as is and then get the other PR #1314 committed over top of it to address the unit tests and implementation details.

If that's the strategy you'd like to take @codeboten, i'll switch mine to an approval then.

[toumorokoshi]

Approved based on the conversation, with merging in unit tests and behavior quickly after.

[codeboten]

Sounds good, let's merge this and get the other PR merged quickly after