Add falcon version 1.4.1 support to opentelemetry-instrumentation-falcon

[rjduffner]

Description

The largest change between falcon 1.x and 2.x was the changing the placement of some exception args. This PR adds some logic to place those args in the correct places for all versions of falcon.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• Made sure tests pass
• Forked and ran against falcon instance internally.

Does This PR Require a Core Repo Change?

• No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[rjduffner]

In falcon 3, not setting remote_addr leads to an error where net.peer.ip is not set in the span.

Between falcon 2 and falcon 3, there was a slightly less than documented change (falconry/falcon#1573) where REMOTE_ADDR is not always set and is therefore not always readable.

Removing the optional remote_addr arg from the simulate_request call (to support falcon 1.4.1) then borked the behavior in falcon 3 (where it used to be totally optional in falcon 2)

https://github.com/falconry/falcon/blob/2.0.0/falcon/testing/helpers.py#L176
vs
https://github.com/falconry/falcon/blob/3.0.1/falcon/testing/helpers.py#L1168-L1172

Function that sets the net.peer.ip var

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/util/opentelemetry-util-http/src/opentelemetry/util/http/httplib.py#L70-L109

[rjduffner]

Given how this is handled in the wsgi instrumentation, I am going to update the tests to make the NET_PEER_IP optional.

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

Lines 195 to 197 in fcba751

	remote_addr = environ.get("REMOTE_ADDR")
	if remote_addr:
	result[SpanAttributes.NET_PEER_IP] = remote_addr

[rjduffner]

Tested locally and working for all three versions, ready for review and approval to run the test suite here.

[ocelotl]

Curious, if remote_addr="127.0.0.1" was removed from the call to simulate_request then why should it appear here? Is this a default value? If so, it would be good to test also a non-default value.

[rjduffner]

@ocelotl,

In falcon 1.4.1 (the new version added in this PR), the remote_addr param doesn't exist. This to me wasn't a huge deal as in the tests, it was being tested with the default value anyways, '127.0.0.1'. So I removed the remote_addr param from the tests.

However in falcon 3.x, there was an update to the code that was made which doesn't set the remote_addr if the default value is used. (See my comment above) or

https://github.com/falconry/falcon/blob/2.0.0/falcon/testing/helpers.py#L176
vs
https://github.com/falconry/falcon/blob/3.0.1/falcon/testing/helpers.py#L1168-L1172

This is different to how it was done in falcon 2.x where the remote_addr was always set, just with the default value.

The if statement in the test here was a compromise to show that there is something strange going on between the different versions.

If you have any thoughts on this, please let me know. I am open to suggestions.

[ocelotl]

Ok, that's fine, can you please add this explanation to the test as a comment? ✌️

[rjduffner]

Added. Thank you

[lzchen]

@owais
PTAL

[rjduffner]

@owais and @lzchen, got everything passing, let me know if there is anything else you would like updated.

[srikanthccv]

Some comments, LGTM.

[rjduffner]

@srikanthccv Not sure what to do here. Lint locally seems to be failing due to this commit, 40c1805

Specifically this

instrumentation/opentelemetry-instrumentation-system-metrics/tests/test_system_metrics.py:25:0: E0611: No name 'InMemoryMetricReader' in module 'opentelemetry.sdk._metrics.export' (no-name-in-module)```


And lint on the runner seems to have a different issue

``` TypeError: __init__() got an unexpected keyword argument 'styles'```

is there a re-run command for lint?

[srikanthccv]

If it's not related to your changes please ignore. We will get that fixed in separate PR.

[rjduffner]

Thanks @srikanthccv

[srikanthccv]

Blocking till we resolve the transitive dep comment

[srikanthccv]

@rjduffner Please add packaging dependency and we are good to merge.

[rjduffner]

@srikanthccv packaging dependency has been added to the setup.cfg.