Repository: ansible/awx
File: awx/main/analytics/core.py (Python)

Code before:
                                if not ship(tgzfile):
                                    slice_succeeded, succeeded = False, False
                                    break

                    if slice_succeeded and collection_type != 'dry-run':
                        with disable_activity_stream():
                            entry = last_entries.get(key)
                            last_entries[key] = max(entry, end) if entry and type(entry) == type(end) else end
                            settings.AUTOMATION_ANALYTICS_LAST_ENTRIES = json.dumps(last_entries, cls=DjangoJSONEncoder)
            except Exception:
                succeeded = False
                logger.exception("Could not generate metric {}".format(filename))

            # update full sync timestamp if successfully shipped
            if full_sync_enabled and collection_type != 'dry-run' and succeeded:
                with disable_activity_stream():
                    last_entries[f"{key}_full"] = now()
                    settings.AUTOMATION_ANALYTICS_LAST_ENTRIES = json.dumps(last_entries, cls=DjangoJSONEncoder)

        if collection_type != 'dry-run':
            # Clean up temp files regardless of success or failure
            for fpath in tarfiles:
                if os.path.exists(fpath):
                    os.remove(fpath)

            # Update timestamps only on success
            if succeeded:
                with disable_activity_stream():
                    if not settings.AUTOMATION_ANALYTICS_LAST_GATHER or until > settings.AUTOMATION_ANALYTICS_LAST_GATHER:
                        settings.AUTOMATION_ANALYTICS_LAST_GATHER = until
                        # `AUTOMATION_ANALYTICS_LAST_GATHER` is set whether collection succeeds or fails;
                        # if collection fails because of a persistent, underlying issue and we do not set last_gather,
                        # we risk the collectors hitting an increasingly greater workload while the underlying issue
                        # remains unresolved. Put simply, if collection fails, we just move on.

                        # All that said, `AUTOMATION_ANALYTICS_LAST_GATHER` plays a much smaller role in determining
                        # what is actually collected than it used to; collectors now mostly rely on their respective entry
                        # under `last_entries` to determine what should be collected.
                        settings.AUTOMATION_ANALYTICS_LAST_GATHER = until

        shutil.rmtree(dest, ignore_errors=True)  # clean up individual artifact files
        if not tarfiles:
            # No data was collected
            logger.warning("No data from {} to {}".format(since or last_gather, until))
            return None

        return tarfiles


def ship(path):
    """

Review comment (bug):
Is this change intentional?:

* previously `AUTOMATION_ANALYTICS_LAST_GATHER` was updated *outside* of that `if succeeded`:
* line 337 still says `AUTOMATION_ANALYTICS_LAST_GATHER is set whether collection succeeds or fails`

So it was documented as intentionally updating in both cases, and it actually did that. Should we be changing this here?

Code after:
                                if not ship(tgzfile):
                                    slice_succeeded, succeeded = False, False
                                    break

                    if slice_succeeded and collection_type != 'dry-run':
                        with disable_activity_stream():
                            entry = last_entries.get(key)
                            last_entries[key] = max(entry, end) if entry and type(entry) == type(end) else end
                            settings.AUTOMATION_ANALYTICS_LAST_ENTRIES = json.dumps(last_entries, cls=DjangoJSONEncoder)
            except Exception:
                succeeded = False
                logger.exception("Could not generate metric {}".format(filename))

            # update full sync timestamp if successfully shipped
            if full_sync_enabled and collection_type != 'dry-run' and succeeded:
                with disable_activity_stream():
                    last_entries[f"{key}_full"] = now()
                    settings.AUTOMATION_ANALYTICS_LAST_ENTRIES = json.dumps(last_entries, cls=DjangoJSONEncoder)

        if collection_type != 'dry-run':
            for fpath in tarfiles:
                if os.path.exists(fpath):
                    os.remove(fpath)

            with disable_activity_stream():
                if not settings.AUTOMATION_ANALYTICS_LAST_GATHER or until > settings.AUTOMATION_ANALYTICS_LAST_GATHER:
                    # `AUTOMATION_ANALYTICS_LAST_GATHER` is set whether collection succeeds or fails;
                    # if collection fails because of a persistent, underlying issue and we do not set last_gather,
                    # we risk the collectors hitting an increasingly greater workload while the underlying issue
                    # remains unresolved. Put simply, if collection fails, we just move on.

                    # All that said, `AUTOMATION_ANALYTICS_LAST_GATHER` plays a much smaller role in determining
                    # what is actually collected than it used to; collectors now mostly rely on their respective entry
                    # under `last_entries` to determine what should be collected.
                    settings.AUTOMATION_ANALYTICS_LAST_GATHER = until

        shutil.rmtree(dest, ignore_errors=True)  # clean up individual artifact files
        if not tarfiles:
            # No data was collected
            logger.warning("No data from {} to {}".format(since or last_gather, until))
            return None

        return tarfiles


def ship(path):
    """
    Ship gathered metrics to the Insights API
    """
    if not path:
        logger.error('Automation Analytics TAR not found')