Skip to content

Add option to auto-create Properties column as DataType SqlXml #130

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

Closed
jducobu opened this issue Aug 2, 2018 · 3 comments · Fixed by #166
Closed

Add option to auto-create Properties column as DataType SqlXml #130

jducobu opened this issue Aug 2, 2018 · 3 comments · Fixed by #166
Assignees

Comments

@jducobu
Copy link

jducobu commented Aug 2, 2018

I work on a console developed in .net core 2.0.
In the appsettings.json file, i have this declaration:

"Serilog": {
    "Using": [ "Serilog.Sinks.MSSqlServer" ],
    "MinimumLevel": "Information",
    "WriteTo": [
      {
        "Name": "MSSqlServer",
        "Args": {
          "connectionString": "Server=.;Database=Serilog;Trusted_Connection=True;MultipleActiveResultSets=true;",
          "tableName": "log",
          "columnOptionsSection": {
            "customColumns": [
              {
                "ColumnName": "EventType",
                "DataType": "int",
                "AllowNull": false
              },
              {
                "ColumnName": "Release",
                "DataType": "varchar",
                "DataLength": 32
              }
            ]
          },
          "autoCreateSqlTable": "true"
        }
      }
    ]
  }

The log table have a properties column's datatype set to nvarchar(max), not xml.

image

A workaround is to exexute this command :

ALTER TABLE [dbo].[log] ALTER COLUMN Properties XML
@MV10
Copy link
Contributor

MV10 commented Aug 2, 2018

I'd strongly disagree with making this a default. Although SQL Server stores the XML data type much more efficiently than the same XML stored as text, it requires extra processing which incurs additional CPU overhead, which potentially reduces write performance -- something you definitely want to avoid when logging. The storage footprint savings are probably irrelevant given that you shouldn't be logging huge data structures. Additionally all of my deployments use JSON for structured data and I'd argue that XML is rapidly falling out of favor.

@MV10
Copy link
Contributor

MV10 commented Aug 30, 2018

I'll tag this as an enhancement request since it comes up periodically. Once I grind through the open bugs, I'm going to start looking at new feature requests.

I think it should be implemented as an option with nvarchar remaining the default.

I'd like to start building up a Wiki for this repo since the README is getting unwieldy, and there I'd caution against the XML type in high-frequency logging scenarios.

@MV10 MV10 added the up-for-grabs This issue waits for a contributor to fix it. label Sep 30, 2018
@MV10 MV10 changed the title The column Properties must be an Xml datatype when autoCreateSqlTable is set to true Add option to auto-create Properties column as DataType SqlXml Oct 7, 2018
@MV10 MV10 removed the up-for-grabs This issue waits for a contributor to fix it. label Oct 9, 2018
@MV10 MV10 self-assigned this Oct 9, 2018
@MV10
Copy link
Contributor

MV10 commented Oct 9, 2018

Currently overhauling configuration and better control over column data types is part of it, including XML column support. Docs will recommend against this for write-performance reasons, most likely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants