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

NLog EcsLayout - Added support for ProcessThreadName + MessageTemplate #348

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Dec 4, 2023

"Original"-field was removed from "Log"-section with #194 . Now trying to restore it as "MessageTemplate" property.

Curious where the recommended place to put the original "MesageTemplate". See also: https://discuss.elastic.co/t/log-original-field-lost-with-upgrade-8-6-1-from-1-5-3/348363

Have tried to follow the Elastic Extensions logging, so "MesageTemplate" becomes additional property. See also: https://www.elastic.co/guide/en/ecs-logging/dotnet/current/extensions-logging-data-shipper.html#_example_document

@snakefoot snakefoot changed the title NLog EcsLayout - Added support for ProcessThreadName NLog EcsLayout - Added support for ProcessThreadName + EventOriginal Dec 4, 2023
@snakefoot snakefoot force-pushed the main branch 2 times, most recently from 904d284 to b29df95 Compare December 4, 2023 21:07
@snakefoot snakefoot changed the title NLog EcsLayout - Added support for ProcessThreadName + EventOriginal NLog EcsLayout - Added support for ProcessThreadName + MessageTemplate Dec 4, 2023
@snakefoot snakefoot force-pushed the main branch 5 times, most recently from e3d858e to 173aefd Compare December 4, 2023 21:48
@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 4, 2023

@Mpdreamz Really confused by this logic in PropDispatch.SetMetaOrLabel:

		public static void SetMetaOrLabel(EcsDocument document, string path, object value)
		{
			switch (value)
			{
				case string s:
					document.Labels ??= new Labels();
					document.Labels[path] = s;
					break;
				case bool b:
					document.Labels ??= new Labels();
					document.Labels[path] = b.ToString(CultureInfo.InvariantCulture).ToLowerInvariant();
					break;
				default:
					document.Metadata ??= new MetadataDictionary();
					document.Metadata[path] = value;
					break;
			}
		}

Why does the field-datatype control whether it is MetaData or Labels ?

Notice NLog Layout has support for explict specifying both:

      <layout xsi:type="EcsLayout">
        <metadata name="MyProperty" layout="MyPropertyValue" /> <!-- repeated, optional -->
        <label name="MyLabel" layout="MyLabelValue" />          <!-- repeated, optional -->
        <tag layout="MyTagValue" />                             <!-- repeated, optional -->
      </layout>

Think it is confusing that if you have explictly specified <metadata>, then output becomes a label because datatype became string/bool.

@Mpdreamz
Copy link
Member

Mpdreamz commented Apr 4, 2024

@snakefoot this is to align with the ECS guidelines around specifying unknown fields: https://www.elastic.co/guide/en/ecs/current/ecs-custom-fields-in-ecs.html#_the_labels_field

labels is explicitly a Dictionary<string, string>
metadata is a .NET extension to store unknown complex fields.

This is a step to move to labels.

I do think labels needs to allow complex fields in the ECS spec and be mapped as https://www.elastic.co/guide/en/elasticsearch/reference/current/flattened.html

However in that case in the long run we might run into a separation of labels into numeric_labels elastic/apm-server#5963.

Will follow up in the ECS repository to discuss long term plans there.

@Mpdreamz
Copy link
Member

Mpdreamz commented Apr 4, 2024

run docs-build

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for implementing this, @snakefoot.

@Mpdreamz Mpdreamz merged commit 1c91c00 into elastic:main Apr 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants