Fix Jaeger exporter with nil values

[drogus]

When there's a nil value in the span data attributes Jaeger exporter will fail with a following error: unexpected error in Jaeger::CollectorExporter#export - Unknown key given to OpenTelemetry::Exporter::Jaeger::Thrift::Tag.new:. It's unexpected and hard to track down as the error does not even give the key name. It's also quite common in Ruby apps to not check for nil values when setting them as an attribute - a common practice is to either conver them to empty strings or ignore them.

This PR ignores nil attributes, but an alternative (empty strings) could also be implemented.

[linux-foundation-easycla]

• ❌ Piotr Sarnacki The commit (4076201) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[ericmustin]

@drogus thanks for the PR here.

I think this generally lgtm, nil values generally shouldnt be allowed in span attributes was my understanding though, https://github.com/open-telemetry/opentelemetry-specification/blob/82865fae64e7b30ee59906d7c4d25a48fe446563/specification/common/common.md#attributes, so i'm curious how we've got to this state.

That being said I think it's good to be defensive here.

I think once you sign the CLA i can approve

[fbogsany]

It happens because we're not checking parameters here:

opentelemetry-ruby/sdk/lib/opentelemetry/sdk/trace/span.rb

Lines 69 to 82 in 659be35

	def set_attribute(key, value)
	super
	@mutex.synchronize do
	if @ended
	OpenTelemetry.logger.warn('Calling set_attribute on an ended Span.')
	else
	@attributes ||= {}
	@attributes[key] = value
	trim_span_attributes(@attributes)
	@total_recorded_attributes += 1
	end
	end
	self
	end

or here:

opentelemetry-ruby/sdk/lib/opentelemetry/sdk/trace/span.rb

Lines 272 to 280 in 659be35

	def trim_span_attributes(attrs)
	return if attrs.nil?
	excess = attrs.size - @trace_config.max_attributes_count
	# TODO: with Ruby 2.5, replace with the more efficient
	# attrs.shift(excess) if excess.positive?
	excess.times { attrs.shift } if excess.positive?
	nil
	end

. #207 is a slightly stagnant PR attempting to address this.

[drogus]

I think this generally lgtm, nil values generally shouldnt be allowed in span attributes was my understanding though,

If that's the case I think a better fix would be to make sure that they're disallowed in the spans, cause my fix is just for the Jaeger exporter. If another exporter also makes the assumption that nil are not allowed it might happen again.

#207 looks interesting, but I see it's quite old, so would it be fine if I created a smaller PR that returns early from set_attribute if a passed value is nil? Not sure if it should do anything else, though. Like - should we raise, warn, log it as a debug message? I'm fine with either I think. Ignoring a nil value would be probably the easiest from the developer's perspective, but could also make it easier for the developer to introduce a bug in their code (like sending a wrong thing, which is nil). So I think my preference would be to ignore and log to DEBUG, but again, I'm fine with any solution that will disallow nils.

[fbogsany]

So I think my preference would be to ignore and log to DEBUG

👍 I think that's good for now. I will be working to improve our error handling story in #465 and we can decide then whether we want to allow devs to optionally raise (e.g. to catch API usage errors in dev).

[drogus]

Sorry for no activity here. I'll work on a fix, but there's also another problem that I've found - it will also fail when the value can't be mapped to one of the thrift values (like any Ruby class other than String and other simple "types"). For now I fixed it on the application level, but I'll also think on a fix for the library, because it's also hard to debug.

[fbogsany]

Sorry for no activity here. I'll work on a fix, but there's also another problem that I've found - it will also fail when the value can't be mapped to one of the thrift values (like any Ruby class other than String and other simple "types"). For now I fixed it on the application level, but I'll also think on a fix for the library, because it's also hard to debug.

It is supposed to only allow:

1. Array of (only one of) String, Float, Integer, boolean
2. String
3. Float
4. Integer
5. boolean

We should log and ignore any other values.

[genebean]

I just ran into this while instrumenting some code and was curious if there has been any progress since the last update.

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot