Change the URLs from HTTP GET to something useful and add the deployment environment for Elastic

[davidgeorgehope]

Proposing this for merge, please let me know what you think.

Thanks

David

Changes

Please provide a brief description of the changes here.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

• CHANGELOG.md updated to document new feature additions
• Appropriate documentation updates in the docs
• Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

[rogercoll]

Thanks for improving the demo. I just added some small comments, please let me know your thoughts. Also, I was wondering if you would like to contribute those changes to the upstream OpenTelemetry-demo repository too, I really think in the community would value the URL attributes decoration (I am able to push those changes too if needed).

[rogercoll]

What do you think of using the URL OTTL function in the transform processor to ensure all cases are covered? Note that this change would also provide semantic convention URL attributes (url.domand and url.path)

  transform:
    error_mode: ignore
    trace_statements:
      - context: span
        statements:
          - merge_maps(attributes, URL(attributes["http.url"]), "upsert")

[davidgeorgehope]

I'll push this upstream but agree this would be useful.

[rogercoll]

Could you provide more context about this change? I think the microservices are setting their corresponding span names, and they should follow the format {method} {format} as defined in https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name

[davidgeorgehope]

@rogercoll actually this is a good point, I was aiming for the guidance in here https://github.com/open-telemetry/opentelemetry-specification/blob/v1.33.0/specification/trace/api.md#span see

get_account | Good, and account_id=42 would make a nice Span attribute
get_account/{accountId} | Also good (using the "HTTP route")

I'll be honest I don't really understand the guidance in the semconv as it seems to contradict this. Honestly though at the moment in elastic the top transaction name becomes "HTTP GET" which is incredibly general and not useful for monitoring imo.

What do you think?

[davidgeorgehope]

To illustrate. This piece of configuration makes the transaction names more specific from this

To this:

imo much more useful. I don't know if the problem is the way the demo is built or the way Elastic interprets the data?

[rogercoll]

Ah, I see. That's a really good point, thanks for raising that. I think the issue is in how the upstream services generate the root spans and which name do they set. The issue seems to only be happening for HTTP requests, GRPC services set a more human-readable name. I have just raised this issue in the upstream repository: open-telemetry#1676

Wdyt if we move this discussion upstream? Using the transform processor in the collector might patch the issue, but I think it would be better to fix it in the root and have a concise name across all generated spans (HTTP and GRPC).

[davidgeorgehope]

Thanks for doing that @rogercoll makes perfect sense.

[rogercoll]

Great!

[davidgeorgehope]

Let me create a new PR with just this change for Elastic since this deployment.environment dependency is very Elastic centric.