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

Improve Error Handling #22

Open
jaredcnance opened this issue Mar 26, 2020 · 3 comments
Open

Improve Error Handling #22

jaredcnance opened this issue Mar 26, 2020 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jaredcnance
Copy link
Member

jaredcnance commented Mar 26, 2020

Problem

There are a few places within the package where logical/configuration errors are swallowed silently (see #19 for one example). We should add a configuration parameter that allows developers to throw validation exceptions in these cases. The original reason for not throwing exceptions was the idea that a monitoring tool shouldn't negatively impact the application. It would be possible that throwing exceptions in these cases would cause applications to crash at runtime. However, this should be weighed against the behavior today which arguably violates the principle of least astonishment. The current behavior is not clear from the interface design.

Next Steps

  • Enumerate all the places where this is an issue.
  • Add configuration parameter THROW_ON_VALIDATION_ERRORS (or better name?) that defaults to false.
  • Accept comments on whether or not the default behavior should be changed to throw which would go out in the next major version.
@jaredcnance jaredcnance added not started enhancement New feature or request good first issue Good for newcomers labels Mar 28, 2020
@heitorlessa
Copy link

I'd vote for raising a ValueError since CloudWatch does not create a metric if Namespace, Dimension, and Metric - Unless that's not the case with EMF...

I agree it can crash an application/function. However, it can be equally frustrating, or damaging depending on the use case, if metrics aren't being created if one forgot a namespace/dimension.

@heitorlessa
Copy link

heitorlessa commented Apr 4, 2020

UPDATE - Updated JSON Schema to require at least one Metric. Discovered while writing tests that I could create an EMF object without a metric

@jaredcnance I noticed the JSON Schema in the docs is wrong, what's the best way to suggest a fix?

Here's a modified version that accounts for:

  • Dimensions: Min of 1 and max of 9
  • Namespace: Must not be empty
  • Metric definitions: Must not be empty
  • Metrics: Min of 1 and Name is a MUST
{
    "type": "object",
    "title": "Root Node",
    "required": [
        "_aws"
    ],
    "properties": {
        "_aws": {
            "$id": "#/properties/_aws",
            "type": "object",
            "title": "Metadata",
            "required": [
                "Timestamp",
                "CloudWatchMetrics"
            ],
            "properties": {
                "Timestamp": {
                    "$id": "#/properties/_aws/properties/Timestamp",
                    "type": "integer",
                    "title": "The Timestamp Schema",
                    "examples": [
                        1565375354953
                    ]
                },
                "CloudWatchMetrics": {
                    "$id": "#/properties/_aws/properties/CloudWatchMetrics",
                    "type": "array",
                    "title": "MetricDirectives",
                    "items": {
                        "$id": "#/properties/_aws/properties/CloudWatchMetrics/items",
                        "type": "object",
                        "title": "MetricDirective",
                        "required": [
                            "Namespace",
                            "Dimensions",
                            "Metrics"
                        ],
                        "properties": {
                            "Namespace": {
                                "$id": "#/properties/_aws/properties/CloudWatchMetrics/items/properties/Namespace",
                                "type": "string",
                                "title": "CloudWatch Metrics Namespace",
                                "examples": [
                                    "MyApp"
                                ],
                                "pattern": "^(.*)$",
                                "minLength": 1
                            },
                            "Dimensions": {
                                "$id": "#/properties/_aws/properties/CloudWatchMetrics/items/properties/Dimensions",
                                "type": "array",
                                "title": "The Dimensions Schema",
                                "minItems": 1,
                                "items": {
                                    "$id": "#/properties/_aws/properties/CloudWatchMetrics/items/properties/Dimensions/items",
                                    "type": "array",
                                    "title": "DimensionSet",
                                    "minItems": 1,
                                    "maxItems": 9,
                                    "items": {
                                        "$id": "#/properties/_aws/properties/CloudWatchMetrics/items/properties/Dimensions/items/items",
                                        "type": "string",
                                        "title": "DimensionReference",
                                        "examples": [
                                            "Operation"
                                        ],
                                        "pattern": "^(.*)$",
                                        "minItems": 1
                                    }
                                }
                            },
                            "Metrics": {
                                "$id": "#/properties/_aws/properties/CloudWatchMetrics/items/properties/Metrics",
                                "type": "array",
                                "title": "MetricDefinitions",
                                "minItems": 1,
                                "items": {
                                    "$id": "#/properties/_aws/properties/CloudWatchMetrics/items/properties/Metrics/items",
                                    "type": "object",
                                    "title": "MetricDefinition",
                                    "required": [
                                        "Name"
                                    ],
                                    "minItems": 1,
                                    "properties": {
                                        "Name": {
                                            "$id": "#/properties/_aws/properties/CloudWatchMetrics/items/properties/Metrics/items/properties/Name",
                                            "type": "string",
                                            "title": "MetricName",
                                            "examples": [
                                                "ProcessingLatency"
                                            ],
                                            "pattern": "^(.*)$",
                                            "minLength": 1
                                        },
                                        "Unit": {
                                            "$id": "#/properties/_aws/properties/CloudWatchMetrics/items/properties/Metrics/items/properties/Unit",
                                            "type": "string",
                                            "title": "MetricUnit",
                                            "examples": [
                                                "Milliseconds"
                                            ],
                                            "pattern": "^(Seconds|Microseconds|Milliseconds|Bytes|Kilobytes|Megabytes|Gigabytes|Terabytes|Bits|Kilobits|Megabits|Gigabits|Terabits|Percent|Count|Bytes\\/Second|Kilobytes\\/Second|Megabytes\\/Second|Gigabytes\\/Second|Terabytes\\/Second|Bits\\/Second|Kilobits\\/Second|Megabits\\/Second|Gigabits\\/Second|Terabits\\/Second|Count\\/Second|None)$"
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

@jaredcnance
Copy link
Member Author

Thanks @heitorlessa! I'll get the docs updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants